Close nodes properly in Coordinator tests #44967
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Today closing a `ClusterNode` in an `AbstractCoordinatorTestCase` uses `onNode()` so has no effect if the node is not in the current list of nodes. It also discards the `Runnable` it creates without having run it, so has no effect anyway. This commit makes these tests much stricter about properly closing the nodes started during `Coordinator` tests, by tracking the persisted states that are opened, and adds an assertion to catch the trappy requirement that the closing node still belongs to the cluster.
DaveCTurner
added
>non-issue
:Distributed/Cluster Coordination
|
I strongly recommend looking at this with whitespace changes hidden. Rather than +1007/-901 it's more like +175/-69. |
DaveCTurner
commented
Jul 29, 2019
| coordinator.stop(); | ||
| clusterService.stop(); | ||
| //transportService.stop(); // does blocking stuff :/ | ||
| clusterService.close(); | ||
| coordinator.close(); | ||
| //transportService.close(); // does blocking stuff :/ | ||
| }); | ||
| }).run(); |
ywelsch
approved these changes
Jul 30, 2019
| final Cluster cluster = new Cluster(randomIntBetween(2, 5)); // 1-node cluster doesn't do any serialization | ||
| cluster.runRandomly(); | ||
| cluster.stabilise(); | ||
| try (Cluster cluster = new Cluster(randomIntBetween(2, 5))) { // 1-node cluster doesn't do any serialization |
There was a problem hiding this comment.
this is not true anymore. Perhaps we should amend this test.
There was a problem hiding this comment.
Good point, this is fine for 1-node clusters too now. Addressed in 92a02b7.
DaveCTurner
deleted the
2019-07-29-clean-up-clusters-in-coordinator-tests
branch
DaveCTurner
added a commit
that referenced
this pull request
Jul 30, 2019
Today closing a `ClusterNode` in an `AbstractCoordinatorTestCase` uses `onNode()` so has no effect if the node is not in the current list of nodes. It also discards the `Runnable` it creates without having run it, so has no effect anyway. This commit makes these tests much stricter about properly closing the nodes started during `Coordinator` tests, by tracking the persisted states that are opened, and adds an assertion to catch the trappy requirement that the closing node still belongs to the cluster.
jkakavas
pushed a commit
that referenced
this pull request
Jul 31, 2019
Today closing a `ClusterNode` in an `AbstractCoordinatorTestCase` uses `onNode()` so has no effect if the node is not in the current list of nodes. It also discards the `Runnable` it creates without having run it, so has no effect anyway. This commit makes these tests much stricter about properly closing the nodes started during `Coordinator` tests, by tracking the persisted states that are opened, and adds an assertion to catch the trappy requirement that the closing node still belongs to the cluster.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Today closing a
ClusterNodein anAbstractCoordinatorTestCaseusesonNode()so has no effect if the node is not in the current list of nodes.It also discards the
Runnableit creates without having run it, so has noeffect anyway.
This commit makes these tests much stricter about properly closing the nodes
started during
Coordinatortests, by tracking the persisted states that areopened, and adds an assertion to catch the trappy requirement that the closing
node still belongs to the cluster.