Sometimes relax lag detector in CoordinatorTests#140434
Merged
DaveCTurner merged 3 commits intoelastic:mainfrom Jan 12, 2026
Merged
Sometimes relax lag detector in CoordinatorTests#140434DaveCTurner merged 3 commits intoelastic:mainfrom
CoordinatorTests#140434DaveCTurner merged 3 commits intoelastic:mainfrom
Conversation
Related to ES-10778: since we're running with a relaxed lag detector in some environments now we really should be covering this in the test suite to verify that the lag detector isn't required for liveness. Relates elastic#108690
Collaborator
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
DaveCTurner
commented
Jan 9, 2026
| assertThat("leader should be last to ack", ackCollector.getSuccessfulAckIndex(leader), equalTo(1)); | ||
|
|
||
| follower0.setClusterStateApplyResponse(ClusterStateApplyResponse.SUCCEED); | ||
| leader.submitValue(randomLong()); // follower0 acks next value allowing cluster to stabilise |
Member
Author
There was a problem hiding this comment.
Without the lag detector and absent any further cluster state updates the lagging node remains lagging, but that's what we want. The point here is that if another cluster state update happens then it stops lagging again.
Comment on lines
+602
to
+604
| cluster.runFor(DEFAULT_STABILISATION_TIME, "allowing new leader election"); | ||
| cluster.getAnyLeader().submitValue(randomLong()); // old leader acks this value allowing cluster to stabilise | ||
| cluster.stabilise(DEFAULT_CLUSTER_STATE_UPDATE_DELAY); |
Member
Author
There was a problem hiding this comment.
Likewise here: stabilising the cluster will not do anything to the lagging node, we need to wait to allow for a master election, then do another cluster state update, to bring the node back in sync.
Contributor
There was a problem hiding this comment.
So if there is a relaxed lag detector, we need to wait for the cluster to stabilise again. If there is not a relaxed lag detector, would we be in this state already?
Member
Author
There was a problem hiding this comment.
DEFAULT_CLUSTER_STATE_UPDATE_DELAY is short, just a few round-trips, so this doesn't add much more time.
In practice DEFAULT_STABILISATION_TIME is long enough for the lag detector to do its thing when it's not in relaxed mode.
Comment on lines
+1815
to
+1818
| .put( | ||
| LagDetector.CLUSTER_FOLLOWER_LAG_TIMEOUT_SETTING.getKey(), | ||
| TimeValue.timeValueMillis(defaultMillis(LagDetector.CLUSTER_FOLLOWER_LAG_TIMEOUT_SETTING)) | ||
| ) |
Member
Author
There was a problem hiding this comment.
This test is the only one that actually needs the lag detector, because we are asserting that it logs what it logs.
| defaultMillis(PUBLISH_TIMEOUT_SETTING) + 2 * DEFAULT_DELAY_VARIABILITY + defaultMillis( | ||
| LagDetector.CLUSTER_FOLLOWER_LAG_TIMEOUT_SETTING | ||
| ) + DEFAULT_DELAY_VARIABILITY + 2 * DEFAULT_DELAY_VARIABILITY, | ||
| DEFAULT_CLUSTER_STATE_UPDATE_DELAY + defaultMillis(PUBLISH_TIMEOUT_SETTING) + 2 * DEFAULT_DELAY_VARIABILITY |
Member
Author
There was a problem hiding this comment.
Timeout here apparently didn't include enough time to do the publication.
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Jan 12, 2026
This test is kinda bogus with an atomic register because it doesn't actually time out as claimed. Nonetheless, we do want to know that the nacks are genuinely delivered eventually under these conditions. This commit adjusts the test to handle the relaxed a lag detector introduced in elastic#140434.
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Jan 12, 2026
This test is kinda bogus with an atomic register because it doesn't actually time out as claimed. Nonetheless, we do want to know that the nacks are genuinely delivered eventually under these conditions. This commit adjusts the test to handle the relaxed a lag detector introduced in elastic#140434. Closes elastic#140509
jimczi
pushed a commit
to jimczi/elasticsearch
that referenced
this pull request
Jan 12, 2026
Related to ES-10778: since we're running with a relaxed lag detector in some environments now we really should be covering this in the test suite to verify that the lag detector isn't required for liveness. Relates elastic#108690
DaveCTurner
added a commit
that referenced
this pull request
Jan 12, 2026
This test is kinda bogus with an atomic register because it doesn't actually time out as claimed. Nonetheless, we do want to know that the nacks are genuinely delivered eventually under these conditions. This commit adjusts the test to handle the relaxed lag detector introduced in #140434. Closes #140509
spinscale
pushed a commit
to spinscale/elasticsearch
that referenced
this pull request
Jan 21, 2026
This test is kinda bogus with an atomic register because it doesn't actually time out as claimed. Nonetheless, we do want to know that the nacks are genuinely delivered eventually under these conditions. This commit adjusts the test to handle the relaxed lag detector introduced in elastic#140434. Closes elastic#140509
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Jan 26, 2026
Today you can set an arbitrarily long timeout on the `LagDetector` but there's no facility to just completely turn it off. The usual values of `0` and `-1` that one might expect to do so are forbidden. There's no good reason for this any more, see e.g. elastic#140434, so with this commit we adjust the setting to accept nonpositive timeouts and interpret them to mean that no lag detection should take place. Relates ES-10778
DaveCTurner
added a commit
to DaveCTurner/elasticsearch
that referenced
this pull request
Jan 26, 2026
Today you can set an arbitrarily long timeout on the `LagDetector` but there's no facility to just completely turn it off. The usual values of `0` and `-1` that one might expect to do so are forbidden. There's no good reason for this any more, see e.g. elastic#140434, so with this commit we adjust the setting to accept nonpositive timeouts and interpret them to mean that no lag detection should take place. Relates ES-10778
DaveCTurner
added a commit
that referenced
this pull request
Jan 27, 2026
Today you can set an arbitrarily long timeout on the `LagDetector` but there's no facility to just completely turn it off. The usual values of `0` and `-1` that one might expect to do so are forbidden. There's no good reason for this any more, see e.g. #140434, so with this commit we adjust the setting to accept nonpositive timeouts and interpret them to mean that no lag detection should take place. Relates ES-10778
schase-es
pushed a commit
to schase-es/elasticsearch
that referenced
this pull request
Jan 28, 2026
Today you can set an arbitrarily long timeout on the `LagDetector` but there's no facility to just completely turn it off. The usual values of `0` and `-1` that one might expect to do so are forbidden. There's no good reason for this any more, see e.g. elastic#140434, so with this commit we adjust the setting to accept nonpositive timeouts and interpret them to mean that no lag detection should take place. Relates ES-10778
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Related to ES-10778: since we're running with a relaxed lag detector in
some environments now we really should be covering this in the test
suite to verify that the lag detector isn't required for liveness.
Relates #108690