Optimize out the version vector specific code (on the client) when the feature is disabled #7482

sbodagala · 2022-06-28T09:27:19Z

Optimize out the version vector specific code on the client (the code that tries to update the version vector on receiving a read version reply from a GRV proxy, and the code in "DatabaseContext::getLatestCommitVersions()") when the version vector feature is disabled.

Testing:
Simulation tests:
Compiler: gcc.
Version vector disabled: 20220629-102752-sre-16a065a2c3175517 (no failures).
Version vector enabled: 20220629-100711-sre-ced0735ad8d1075d (no failures).

Code-Reviewer Section

The general 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)

when the version vector feature is disabled.

sbodagala · 2022-06-28T09:50:54Z

fdbclient/DatabaseContext.h

 	// Cache of the latest commit versions of storage servers.
 	VersionVector ssVersionVectorCache;

+	// Introduced mainly to optimize out the version vector related code (on the client side)


Shall we propagate the version vector knob enable/disable information, from GRV proxy to client, in "GetReadVersionReply" message and use that information in place of using this check? But I think that idea may not work correctly on recovery boundaries (because "GetReadVersionReply" messages may cross recovery boundaries). We can explore that idea further if we think that is a better approach than using this check.

it seems returning vv status from the GRV request would be better- Can the client use the fact that new GRV proxies were created (monitorClientDBInfoChange) as a signal that recovery happened?

I'd prefer removing mayNeed from the function name. It sounds ambiguous.. perhaps rename to versionVectorCacheActive()

can you make this an inline function for speed

it seems returning vv status from the GRV request would be better-

I thought more about it. This might not work if the knob was turned on before recovery and turned off after that. The version vector cache (on the client) won't get cleared after recovery and reads could reference a stale latest commit version from the version vector.

fdb-windows-ci · 2022-06-28T10:00:48Z

Doxense CI Report for Windows 10

Commit ID: 0da13a7
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-06-28T10:06:24Z

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Commit ID: 0da13a7
Duration 0:38:55
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-06-28T10:08:36Z

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

Commit ID: 0da13a7
Duration 0:41:06
Result: ❌ FAILED
Error: Error while executing command: tar -czf "${CODEBUILD_SRC_DIR}/packaging/docker/website/${FDB_VERSION}/fdb_${FDB_VERSION}.tar.gz" --directory "${CODEBUILD_SRC_DIR}/packaging/docker/website/${FDB_VERSION} .. Reason: exit status 2
Build Logs (available for 30 days)

foundationdb-ci · 2022-06-28T10:21:32Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 0da13a7
Duration 0:54:03
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

- Optiize out the code in DatabaseContext::getLatestCommitVersions() in the case where the feature is turned off.

foundationdb-ci · 2022-06-29T10:57:59Z

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Commit ID: a48008f
Duration 0:39:09
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-06-29T10:59:17Z

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

Commit ID: a48008f
Duration 0:40:27
Result: ❌ FAILED
Error: Error while executing command: tar -czf "${CODEBUILD_SRC_DIR}/packaging/docker/website/${FDB_VERSION}/fdb_${FDB_VERSION}.tar.gz" --directory "${CODEBUILD_SRC_DIR}/packaging/docker/website/${FDB_VERSION} .. Reason: exit status 2
Build Logs (available for 30 days)

foundationdb-ci · 2022-06-29T11:14:55Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: a48008f
Duration 0:56:00
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

dlambrig · 2022-06-29T15:19:50Z

fdbclient/DatabaseContext.h

+	// Introduced mainly to optimize out the version vector related code (on the client side)
+	// when the version vector feature is disabled (on the server side).
+	// @param ssVersionVectorDelta version vector changes sent by GRV proxy
+	inline bool versionVectorCacheActive(const VersionVector& ssVersionVectorDelta) {


it looks like the version vector constructor initializes maxVersion to invalidVersion for ssVersionVectorCache. From that point forward, the PR makes things such that we don't allow ssVersionVectorCache to be modified with clear() or applyDelta() if maxVersion is set to invalidVersion.

So ssVersionVectorCache will never be set? I may be misunderstanding the code.. !

From that point forward, the PR makes things such that we don't allow ssVersionVectorCache to be modified with clear() or applyDelta() if maxVersion is set to invalidVersion.

Not quite. We don't allow ssVersionVectorCache to be modified if (a) maxVersion is set to invalidVersion and (b) the version vector delta received from the GRV also has maxVersion set to invalidVersion. Basically, invoking "clear()" or "applyDelta()" won't have any effect in this case.

jzhou77 · 2022-06-30T16:23:35Z

fdbclient/NativeAPI.actor.cpp

+						if (cx->isCurrentGrvProxy(v.proxyId)) {
+							cx->ssVersionVectorCache.applyDelta(v.ssVersionVectorDelta);
+						} else {
+							continue; // stale GRV reply, retry


Unrelated to this change, this line can potentially become a busy loop. Maybe we should add a backoff here in a separate PR.

On "continue" another "GetReadVersionRequest" gets sent, and wouldn't it wait on the "choose" statement after that? Trying to understand how it will become a busy loop. But maybe we can discuss that offline.

foundationdb-ci · 2022-07-01T05:34:32Z

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

Commit ID: 037c06a
Duration 0:41:03
Result: ❌ FAILED
Error: Error while executing command: tar -czf "${CODEBUILD_SRC_DIR}/packaging/docker/website/${FDB_VERSION}/fdb_${FDB_VERSION}.tar.gz" --directory "${CODEBUILD_SRC_DIR}/packaging/docker/website/${FDB_VERSION} .. Reason: exit status 2
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-01T05:35:06Z

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Commit ID: 037c06a
Duration 0:41:35
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-01T05:56:18Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 037c06a
Duration 1:02:50
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

Optimize out the version vector specific code (on the client) when the feature is disabled

- Optimize out the version vector specific code (on the client)

0da13a7

when the version vector feature is disabled.

sbodagala requested review from dlambrig and jzhou77 June 28, 2022 09:27

sbodagala commented Jun 28, 2022

View reviewed changes

sbodagala requested review from vishesh June 28, 2022 18:09

sbodagala added 2 commits June 29, 2022 10:13

- Address a review comment

c74710f

- Optiize out the code in DatabaseContext::getLatestCommitVersions() in the case where the feature is turned off.

- Address a review comment

a48008f

dlambrig reviewed Jun 29, 2022

View reviewed changes

dlambrig approved these changes Jun 29, 2022

View reviewed changes

jzhou77 approved these changes Jun 30, 2022

View reviewed changes

Merge remote-tracking branch 'apple-upstream/main'

037c06a

sbodagala merged commit 89a5eeb into apple:main Jul 1, 2022

sbodagala added a commit to sbodagala/foundationdb that referenced this pull request Jul 6, 2022

Merge pull request apple#7482 from sbodagala/main

c6d541a

Optimize out the version vector specific code (on the client) when the feature is disabled

sbodagala mentioned this pull request Jul 6, 2022

Cherry pick pull request #7482 #7528

Merged

5 tasks

jzhou77 pushed a commit that referenced this pull request Jul 6, 2022

Merge pull request #7482 from sbodagala/main

3f1b658

Optimize out the version vector specific code (on the client) when the feature is disabled

Optimize out the version vector specific code (on the client) when the feature is disabled #7482

Optimize out the version vector specific code (on the client) when the feature is disabled #7482

Uh oh!

Conversation

sbodagala commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code-Reviewer Section

For Release-Branches

Uh oh!

sbodagala Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

dlambrig Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbodagala Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

sbodagala Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdb-windows-ci commented Jun 28, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Jun 28, 2022

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Uh oh!

foundationdb-ci commented Jun 28, 2022

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

Uh oh!

foundationdb-ci commented Jun 28, 2022

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

foundationdb-ci commented Jun 29, 2022

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Uh oh!

foundationdb-ci commented Jun 29, 2022

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

Uh oh!

foundationdb-ci commented Jun 29, 2022

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

dlambrig Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbodagala Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzhou77 Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

sbodagala Jun 30, 2022

Choose a reason for hiding this comment

Uh oh!

foundationdb-ci commented Jul 1, 2022

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

Uh oh!

foundationdb-ci commented Jul 1, 2022

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Uh oh!

foundationdb-ci commented Jul 1, 2022

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sbodagala commented Jun 28, 2022 •

edited

Loading

dlambrig Jun 28, 2022 •

edited

Loading

sbodagala Jun 29, 2022 •

edited

Loading

dlambrig Jun 29, 2022 •

edited

Loading

sbodagala Jun 29, 2022 •

edited

Loading