Fix race condition in InternalTestCluster.getMasterName()
#127985
Conversation
Prevents assertion errors when the master node is null after obtaining the cluster state in the second call. See PRs 127213 and 127891 for details about recent changes to this method, and where the code used in this commit was proposed.
JeremyDahlgren
added
>test
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
JeremyDahlgren
requested review from
DaveCTurner,
nielsbauman,
pxsalehi and
ywangd
|
What exactly would be the value of this change? I understand that we're trying to avoid test failures where tests use IMO, it makes sense that I think #127891 was valuable in changing the exception message to something a lot more understandable. Other than adding documentation to this method that it shouldn't be used in unstable clusters, I don't immediately see much more we can do to avoid people from using this method in unstable clusters. |
|
Yeah I agree with Niels, it's trappy if Instead, if a test expects (i.e. causes) the cluster to be unstable, it should be explicitly waiting for stabilisation before starting to think about identifying the master. |
No value, will close, thank you and David for your clear explanations. I will reference the original comment from when this issue was discusssed in 127213 and this PR when needed. |
4 participants
Prevents assertion errors when the master node is null after obtaining the cluster state in the second call.
See PRs #127213 and #127891 for details about recent changes to this method, and where the code used in this commit was proposed.
Per #127213
getMasterName()was deemed unsafe to call in unstable clusters and scenarios where there could be election jitter. Updating the javadoc to indicate the stable cluster requirement for these methods would have been a good addition, but it likely would not prevent future usages of the methods in such unstable test scenarios. The utility method added in serverless PR 3833 is an example of code added after #127213, and led to a failing test seen in serverless issue 3867. It was suggested in the PR under review for that issue that the root cause of the NPE should be fixed.There have also been other recent failing tests where the PRs to resolve them required refactoring to avoid the use of
getMasterName(), serverless issue 3865 and #127523. It is likely we'll run into more test failures, there currently are 233 calls togetMasterName(). I haven't gone through all of them to see under what cluster conditions it is being called. It might make sense to apply David's suggested change (submitted in this PR) to avoid future failures. Note that if the current cluster state satisfies the predicate (a non-null master node) the listener will be completed immediately.