Decouple the TransportService and ClusterService #16872
Conversation
|
@s1monw I still want to add a unit test for the new NodeConnectionsService but I think we can start the review process and get feedback.. |
| assert localNode == null || localNode.equals(event.state().nodes().localNode()); | ||
| localNode = event.state().nodes().localNode(); | ||
|
|
||
| // nocommit: this can be done in parallel using parallel stream but then we get the following permission |
There was a problem hiding this comment.
@rmuir can you take a look at this nocommit - how big a worry is it that we now can't do parallel streams?
There was a problem hiding this comment.
It is no concern at all, it only prevents deadlocks and performance issues :)
Don't use parallel streams with the default pool, specify a pool instead (in the same or child thread group):
http://blog.krecan.net/2014/03/18/how-to-specify-thread-pool-for-java-8-parallel-streams/
There was a problem hiding this comment.
Thanks for educating me. Creating a ForkJoinPool seems like an overkill here (especially as I can't find an easy way to reuse our ThreadPools). I'll leave it as is (and re-add orignal todo to parallelize it - we can still do it "old style" if we want).
|
left some intial review comments |
bleskes
force-pushed
the
decouple_cluster_service_transport
branch
from
0a7ab96 to
7eb3724
Compare
|
@s1monw I rebased, added a unit test and addressed the comments. I think it's ready for review.. |
|
|
||
| void validateNodeConnected(DiscoveryNode node) { | ||
| assert nodeLocks.isHeldByCurrentThread(node) : "validateNodeConnected must be called under lock"; | ||
| if (lifecycle.stoppedOrClosed()) { |
There was a problem hiding this comment.
any chance that this method has only one return statement?
There was a problem hiding this comment.
maybe combine all the empty ifs in a single one ?
I agree with your plan here lets fix it in several steps and be explicit for now... |
|
lets go with what we have I added some minors but LGTM otherwise, it's progress |
|
@s1monw thx. pushed another commit |
|
LGTM thanks @bleskes |
bleskes
force-pushed
the
decouple_cluster_service_transport
branch
from
54d6a90 to
cd12241
Compare
Currently, the cluster service is tightly coupled to the transport service by both managing node connections and requiring the bound address in order to create the local disco node. This commit introduces a new NodeConnectionsService which is in charge of node connection management and makes it possible to remove all network related calls from the cluster service. The local DiscoNode is now created by DiscoveryNodeService and is set both the cluster service and the transport service during node start up. Closes elastic#16788 Closes elastic#16872
Currently, the cluster service is tightly coupled to the transport service by both managing node connections and requiring the bound address in order to create the local disco node. This commit introduces a new NodeConnectionsService which is in charge of node connection management and makes it possible to remove all network related calls from the cluster service. The local DiscoNode is now created by DiscoveryNodeService and is set both the cluster service and the transport service during node start up.
Closes #16788
Note that this simplifies constructing the InternalClusterService making a viable option for testing. I already moved some of its testing to a unit test. As a follow up, I will attempt removing all test implementations of ClusterService and move InternalClusterService to be the one and only ClusterService.