Improvements to the unicast-based recovery version computation algorithm #11770
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
the following ways: - Make it use "max(KCV)", instead of "min(KCV)", as the default recovery version - Make it return the <max(KCV), recovery version> pair as output - Make the invoker use the "max(KCV)" version returned by the algorithm - Address a bug related to the computation of recovery version (in the case where a subset of log servers have empty "unknownCommittedVersions" list) And, make the invoker compute the max() and min() of recovery versions of primary and satellite DCs based on the recovery versions computed by the appropriate (with/without unicast) recovery version computation algorithms.
|
Please review the changes and comment on whether this PR can be merged into "main". If we can agree these changes are not disruptive and can be merged into "main" then I will take it out of the "Draft" status, thanks! |
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux CentOS 7
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux CentOS 7
|
Result of foundationdb-pr on Linux CentOS 7
|
dlambrig
approved these changes
Nov 21, 2024
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.
Modify the unicast-based recovery version computation algorithm the following ways:
And, make the invoker compute the max() and min() of recovery versions of primary and satellite DCs based on the recovery versions computed by the appropriate (with/without unicast) recovery version computation algorithms.
Testing:
Joshua job (with version vector feature disabled): 20241111-215149-sre-3f2cb6906ac4ea16 (no failures).
More details:
How can the "KnownCommittedVersion" advance after a log server/a subset of log servers are locked when unicast is enabled? This can happen because a commit version may get sent only to a subset of log servers when unicast is enabled and so even after a log server is/a subset of log servers are locked the remaining log servers could receive a version, commit that version, and that could cause the known committed version to advance.
Consider log servers S_a, S_b, S_c, S_d, and RF = 1. Suppose servers S_a and S_b got locked at version V_x, and known committed version on those servers be K_x (K_x < V_x). Now servers S_c and S_d could receive another version V_y (V_x < V_y) before they receive the lock message from the cluster controller, could commit V_y, could advance the known committed version to V_y on the proxy, and could receive V_y as the known committed version before they receive the lock request.
Here is a scenario from a simulation test that was causing a wrong recovery version to be selected because the known committed version was advancing after a log server is locked and we were using "min(kcv)" as the default recovery version:
Log server interfaces and their IDs:
Interface: b5c90942058a7b8c, ID: 0
Interface: ee2db35be3e7caf7, ID: 1
Interface: 727dc80e10ce7857, ID: 2
Versions and the log servers those versions were sent to:
Version 47631442 sent to and received by log server with ID: 1 and sent to and received by log server with ID: 0.
Version 49130513 sent to and received by log server with ID: 2) and sent to and received by log server with ID: 0.
Version 49771720 sent to and received by log server with ID: 2 and sent to but not received by log server with ID: 1.
Version 50985880 sent to but not received by log server with ID: 1 and sent to and received by log server with ID: 0.
Log servers with IDs 0 and 1 are reporting to cluster controller during recovery.
Log server with ID 1 gets stopped/locked at version 47631442, has known committed version 45781955 and reports unknown committed version list: 47631442
Log server with ID 0 gets stopped/locked at version 50985880, has known committed version 49130513 and reports unknown committed version list: 50985880
Note that the known committed version got advanced on log server with ID 0 after log server with ID 1 got locked.
Since the known committed version got advanced on ID 1 it won't report versions below 49130513 as unknown committed versions.
If the recovery algorithm uses "min(KCV)" as the default recovery version it will report 47631442 as the recovery version which is wrong. Using "max(KCV)" will make the algorithm pick the correct recovery version 49130513.
Code-Reviewer Section
The general pull request guidelines can be found here.
Please check each of the following things and check all boxes before accepting a PR.
For Release-Branches
If this PR is made against a release-branch, please also check the following:
release-branchormainif this is the youngest branch)