Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Track shard moves for version vector #11977

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Track shard moves for version vector #11977

wants to merge 3 commits into from

Conversation

dlambrig
Copy link
Contributor

@dlambrig dlambrig commented Feb 27, 2025 •
edited
Loading

With version vector, store the most recent version that modified the shard map on the resolvers, and include this value on replies back to the CP. On commits, the CP sends to the RV what it thinks is the most recent version of a shard change. If the CP is behind the RV, it is unaware of the latest shard change and may have the wrong set of tags; send the commit to all logs.

scenario

  1. Two transactions on the same CP access the same shard map.
  2. Transaction 1: Modifies the shard map on the resolver and then completes, updating the shard map on the CP.
  3. Transaction 2: Before Transaction 1 returns from the resolver, reads tags for the shard using the old shard map (pre-resolution).
  4. Transaction 1 completes, now the shard map is updated on the CP.
  5. Transaction 2 uses the outdated tags (from the old shard map) to get locations from the resolver (same RV as transaction 1).
  6. Transaction 2 returns and accesses the updated shard map on the CP, leading to a mismatch between the chosen shard and the previously retrieved tags.

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.

  • The PR has a description, explaining both the problem and the solution.
  • The description mentions which forms of testing were done and the testing seems reasonable.
  • Every function/class/actor that was touched is reasonably well documented.

For Release-Branches

If this PR is made against a release-branch, please also check the following:

  • This change/bugfix is a cherry-pick from the next younger branch (younger release-branch or main if this is the youngest branch)
  • There is a good reason why this PR needs to go into a release branch and this reason is documented (either in the description above or in a linked GitHub issue)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-ide on Linux CentOS 7

  • Commit ID: dbf1126
  • Duration 0:24:19
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: dbf1126
  • Duration 0:38:33
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: dbf1126
  • Duration 0:48:41
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: dbf1126
  • Duration 0:51:39
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: dbf1126
  • Duration 1:00:34
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: dbf1126
  • Duration 1:14:06
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@dlambrig dlambrig marked this pull request as ready for review March 5, 2025 17:18
@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-ide on Linux CentOS 7

  • Commit ID: 6410831
  • Duration 0:20:23
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

jzhou77
jzhou77 approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@jzhou77 jzhou77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

fdbserver/Resolver.actor.cpp
@@ -88,8 +88,7 @@ class RecentStateTransactionsInfo {
// sends this request has them and will apply them via applyMetadataToCommittedTransactions();
// and other proxies will get this version's state txns as a prior version.
for (; stateTransactionItr != endItr; ++stateTransactionItr) {
shardChangedOrStateTxn =
shardChangedOrStateTxn || stateTransactionItr->value.first || stateTransactionItr->value.second.size();
shardChangedOrStateTxn = shardChangedOrStateTxn || stateTransactionItr->value.first;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this line change fix a bug?

maybe when shardChangedOrStateTxn is false, we can just continue?

Copy link
Contributor Author

@dlambrig dlambrig Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes it more efficient, we do not need to send to all tLogs on the local CP if another CP modified the transaction state store- the other CP will commit the change.

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x

  • Commit ID: 6410831
  • Duration 0:38:34
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang-arm on Linux CentOS 7

  • Commit ID: 6410831
  • Duration 0:47:12
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-cluster-tests on Linux CentOS 7

  • Commit ID: 6410831
  • Duration 0:59:27
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-macos on macOS Ventura 13.x

  • Commit ID: 6410831
  • Duration 1:03:26
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr-clang on Linux CentOS 7

  • Commit ID: 6410831
  • Duration 1:05:15
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci
Copy link
Contributor

Result of foundationdb-pr on Linux CentOS 7

  • Commit ID: 6410831
  • Duration 1:10:26
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

sbodagala
sbodagala reviewed Mar 5, 2025
View reviewed changes
fdbserver/CommitProxyServer.actor.cpp
@@ -861,6 +865,7 @@ std::set<Tag> CommitBatchContext::getWrittenTagsPreResolution() {
toCommit.storeRandomRouterTag();
transactionTags.insert(toCommit.savedRandomRouterTag.get());
}
lastShardMove = pProxyCommitData->lastShardMove;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a timing issue here? The last shard move version would need to be obtained at the time the transaction gets started/the time the tags get collected? I think it should be at the point where we start accessing the shard metadata in this transaction? Else, the last shard move version could get updated in this window and we might not report the correct version to the resolver?

sbodagala
sbodagala reviewed Mar 5, 2025
View reviewed changes
fdbserver/include/fdbserver/ProxyCommitData.actor.h
@@ -277,6 +277,8 @@ struct ProxyCommitData {
std::shared_ptr<AccumulativeChecksumBuilder> acsBuilder = nullptr;
LogEpoch epoch;

Version lastShardMove;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this getting populated please? Is that done in a different commit?

Copy link
Contributor Author

@dlambrig dlambrig Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the constructor initializes it to invalidVersion. But it should be updated on return from the resolver, this line was dropped when I ported by debug patch. I'll update the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbodagala sbodagala sbodagala left review comments

@jzhou77 jzhou77 jzhou77 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dlambrig @foundationdb-ci @jzhou77 @sbodagala