Remove Custom...Server.java classes and replace with Thrift implementation - #6454
Remove Custom...Server.java classes and replace with Thrift implementation#6454Amemeda wants to merge 17 commits into
Conversation
ctubbsii
left a comment
There was a problem hiding this comment.
I need to check this over again to see if there's anything else, but I did find one thing.
|
I think |
ctubbsii
left a comment
There was a problem hiding this comment.
Looks good, but a few thoughts:
- Still some room to clean up boilerplate to reduce redundant setting of the event handler
- I am very wary of relying on the ThreadLocal. I think code that uses the clientAddress should really make sure to grab it right after the initial RPC connection, and pass it along, so that way they know they are grabbing it from the same thread in which it is set; I skimmed all the times the thread local was accessed, and it seemed like it was only being done in the same thread where it is set, but it's hard to verify because in many places, we only use it when an exception occurs.
- Since the enum string was altered, backwards compatibility needs to be fixed if a user explicitly set "custom_hs_ha" in their config instead of relying on the defaults
- I didn't actually run it to verify that the trace log messages or the audit logs were recording the client address. Please check that and verify that it is still logging correctly.
Of these, 3 and 4 are the most important. 1 and 2 could be done as a follow on. Of those, 1 is the more important one for me, since its smaller and would simplify the utility class a bit, making the code slightly more maintainable (which is important for future work).
|
It looks like some tests aren't working with these changes. I haven't dug into why yet. |
|
Not sure if its related, but #6491 is also timing out on a mini cluster test |
|
I reran the failing job and its passing now. Not sure what caused the failures though |


Investigated relevance of a few custom tserver related java classes, deleted them, reworked their implementation with thrift methods
Result:
setServerEventHandler(new ThriftServerEventHandler());to applicable tserversThriftServerType.CUSTOM_HS_HA, updated usages