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 11 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.
| } | ||
|
|
||
| return new ServerAddress(new CustomNonBlockingServer(options), address); | ||
| final THsHaServer server = new THsHaServer(options); |
There was a problem hiding this comment.
In e2efd04, we added an override to the stop method because the upstream implementation does not attempt to shut down its internal executor service when stop is requested. In fact, it previously existed using reflection, but this commit cleaned up the implementation.
I think we probably still need that here, although, we don't need to override the whole class. We can probably just do it inline here, something like:
| final THsHaServer server = new THsHaServer(options); | |
| final THsHaServer server = new THsHaServer(options) { | |
| @Override | |
| public void stop() { | |
| super.stop(); | |
| try { | |
| getInvoker().shutdownNow(); | |
| } catch (Exception e) { | |
| log.error("Unable to call shutdownNow", e); | |
| } | |
| } | |
| }; |
This is probably a bugfix we can contribute to the Thrift project, so we don't need to maintain this indefinitely, but I think we still want it for now.
There was a problem hiding this comment.
I started looking into submitting a patch upstream to Thrift, and some of the code they have there seems worryingly bad. They have a graceful shutdown method for the invoker, but they never call it. It also uses awaitTermination but doesn't check the return boolean, and the loop uses the system clock rather (currentTimeMillis) than the CPU clock (nanoTime).
Upstream also allows the executor to be passed in as an argument, so you can provide a different implementation. But also that allows you to manage its lifecycle outside of this server's lifecycle, but I'm not sure that's a great idea.
In short, I think it's fine to keep our patch for now. Submitting a patch upstream isn't straight-forward... as it's not clear what the right design should be.
|
I think |
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