Cleanup Transport*VotingConfigExclusionsActionTests #98146
Conversation
elasticsearchmachine
added
needs:triage
| @@ -170,11 +168,11 @@ public void testWithdrawsVoteFromANode() throws InterruptedException { | |||
| }) | |||
| ); | |||
|
|
|||
| assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); | |||
| safeAwait(countDownLatch); | |||
| assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(), contains(otherNode1Exclusion)); | |||
There was a problem hiding this comment.
You've moved some of the assertions about the current state into the success handler - I would slightly prefer them all to move, but whichever way we go I'd rather we were consistent.
There was a problem hiding this comment.
This comment is still outstanding.
There was a problem hiding this comment.
Apologies, I had just taken a quick look at the merge conflicts, I hadn't meant to give impression I had addressed the issues you raised. I should have made my intentions more explicit.
I was keeping some of the assertions outside the handler because I'd, superficially, made the assumption the handler must be being called twice in order to decrement the CountDownLatch twice, so the assertion would need to be outside the handler to prevent it being called twice also. I can see this isn't the case now, so have moved the remaining assertions as well.
DaveCTurner
added
:Distributed Coordination/Cluster Coordination
elasticsearchmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this comment.
Thanks for the updates @alyokaz, however this now doesn't even compile (or pass ./gradlew precommit).
| @@ -170,11 +168,11 @@ public void testWithdrawsVoteFromANode() throws InterruptedException { | |||
| }) | |||
| ); | |||
|
|
|||
| assertTrue(countDownLatch.await(30, TimeUnit.SECONDS)); | |||
| safeAwait(countDownLatch); | |||
| assertThat(clusterService.getClusterApplierService().state().getVotingConfigExclusions(), contains(otherNode1Exclusion)); | |||
There was a problem hiding this comment.
This comment is still outstanding.
Move assertions into success handlers. Replace call to await on CoundDownLatch with safeAwait.
alyokaz
force-pushed
the
refactor-98057-clean-up-transport-voting-config-exclusion-action-tests
branch
from
725ac65 to
7b2b127
Compare
|
I've ran |
|
@elasticmachine test this please |
|
Happy to contribute @DaveCTurner, thanks for the review |
The docs for this API say the following: > If the API fails, you can safely retry it. Only a successful response > guarantees that the node has been removed from the voting > configuration and will not be reinstated. Unfortunately this isn't true today: if the request adds no exclusions then we do not wait before responding. This commit makes the API wait until all exclusions are really applied. Backport of elastic#98386, plus the test changes from elastic#98146 and elastic#98356.
The docs for this API say the following: > If the API fails, you can safely retry it. Only a successful response > guarantees that the node has been removed from the voting > configuration and will not be reinstated. Unfortunately this isn't true today: if the request adds no exclusions then we do not wait before responding. This commit makes the API wait until all exclusions are really applied. Backport of #98386, plus the test changes from #98146 and #98356.
This pull request fixes #98057
The assertions have been moved into the handlers where they can be, and the calls to
countDownLatch.await()as been replaced bysafeAwait.Attempting to replace
TransportResponseHandlerwithActionListenerResponseHandlerseems to be less concise due to the need to implement anActionListenerfor its constructor, which then requires the introduction of a new method to keep things DRY.