-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Always send coordination metadata in remote state diff #16215
base: main
Are you sure you want to change the base?
Conversation
8798069
to
3a1e37f
Compare
❌ Gradle check result for 3a1e37f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only concern I see is the additional download overhead(remote store calls) on the follower nodes
I think the long term fix should be to update the lastSeen cluster state in commit phase with the committed config. |
Signed-off-by: Sooraj Sinha <[email protected]>
3a1e37f
to
3826aa7
Compare
❌ Gradle check result for 3826aa7: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
private void updateLastSeen(final ClusterState clusterState) { | ||
lastSeenClusterState.set(clusterState); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Would prefer inlining it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, I am assuming last seen that we are setting is at the end of the publication and no failure can revert this last seen for the given term and version
Description
Coordination metadata contains accepted voting configuration and committed voting configuration.
When the voting configuration changes, the changed accepted voting configuration is sent in publish phase. The follower nodes apply this publish phase cluster state on the
lastSeen
cluster state in PublicationTransportHandler. In the commit phase, each node set the committed voting configuration using the accepted voting configuration by themselves.When the next cluster state is published, it does not contain any diff in the voting configuration. This next cluster state update uses
lastSeen
again to apply the diff due to which the committed voting configuration is still set to an older value.With local cluster state publication, this issue does not occur as the entire coordination metadata is sent in every diff.
With remote cluster state publication, we can mitigate this in the same way by always sending coordination metadata in the diff.
Related Issues
NA
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.