Track when version not found in version vector. by dlambrig · Pull Request #7653 · apple/foundationdb

dlambrig · 2022-07-20T22:38:41Z

Add new counter to track when the commit version for an SS is not taken from the version vector. When version vector is enabled there may be a performance hit in that situation. Performance diagnosis is harder without it. The counter should normally be 0.

Joshua:
20220728-195222-henrylambright-45953926d0fe9fe5

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)

dlambrig · 2022-07-20T22:42:55Z

 	}

-	if (ssVersionVectorCache.getMaxVersion() != invalidVersion && readVersion > ssVersionVectorCache.getMaxVersion()) {
+	if (readVersion > ssVersionVectorCache.getMaxVersion()) {


this check was redundant per line 241 above.

This was my bad, actually. I should have removed this redundant check as part of commit c74710f.

fdb-windows-ci · 2022-07-20T22:57:55Z

Doxense CI Report for Windows 10

Commit ID: 82624e6
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-20T23:37:57Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 82624e6
Duration 0:59:06
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-21T02:56:40Z

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

Commit ID: 82624e6
Duration 4:17:50
Result: ❌ FAILED
Error: Build has timed out.
Build Logs (available for 30 days)

fdb-windows-ci · 2022-07-21T15:09:38Z

Doxense CI Report for Windows 10

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

foundationdb-ci · 2022-07-21T15:32:58Z

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

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

foundationdb-ci · 2022-07-21T15:51:37Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: b01e8b8
Duration 1:01:17
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Logs (available for 30 days)

sbodagala · 2022-07-21T18:06:21Z

 		if (!updatedVersionMap) {
 			TraceEvent(SevDebug, "CommitVersionNotFoundForSS")
-			    .detail("InSSIDMap", ssidTagMapping.find(uid) != ssidTagMapping.end() ? 1 : 0)
+			    .detail("InSSIDMap", iter != ssidTagMapping.end() ? 1 : 0)


I am concerned about the overhead that would be imposed by this event - Is this trace event really needed? What additional actionable information does this provide in production?

I think we only log SevInfo and higher in production. There is a knob controlling the logging level.

It is SevDebug, so off by default. I think it is useful to have if we see a problem in ckperf that is hard to reproduce elsewhere. In that case we may wish to change the trace severity knob to Debug level (this seems to be a common code pattern in fdb), and can find which of the 3 conditionals is failing.

Ok. Would you like to log the contents of the version vector too (this may help with identifying special scenarios like post-recovery at which point the version vector is going to be empty)?

foundationdb-ci · 2022-07-21T19:07:06Z

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

Commit ID: b01e8b8
Duration 4:16:47
Result: ❌ FAILED
Error: Build has timed out.
Build Logs (available for 30 days)

jzhou77 · 2022-07-21T18:31:04Z

 		if (!updatedVersionMap) {
 			TraceEvent(SevDebug, "CommitVersionNotFoundForSS")
-			    .detail("InSSIDMap", ssidTagMapping.find(uid) != ssidTagMapping.end() ? 1 : 0)
+			    .detail("InSSIDMap", iter != ssidTagMapping.end() ? 1 : 0)


I think we only log SevInfo and higher in production. There is a knob controlling the logging level.

fdb-windows-ci · 2022-07-22T15:09:51Z

Doxense CI Report for Windows 10

Commit ID: 850a59b
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-22T15:25:41Z

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Commit ID: 850a59b
Duration 0:42:34
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-22T15:43:13Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 850a59b
Duration 1:00:03
Result: ❌ FAILED
Error: Error while executing command: if python3 -m joshua.joshua list --stopped | grep ${ENSEMBLE_ID} | grep -q 'pass=10[0-9][0-9][0-9]'; then echo PASS; else echo FAIL && exit 1; fi. Reason: exit status 1
Build Logs (available for 30 days)

jzhou77 · 2022-07-22T16:28:15Z

+			    .detail("Tag", tag)
+			    .detail("CommitVersion", commitVersion)
+			    .detail("ReadVersion", readVersion)
+			    .detail("VersionVector", ssVersionVectorCache.toString());


This might be large when VV has many entries and the value may be truncated. So probably need to set the max field and event length.

I will use settings I see many places elsewhere. .setMaxEventLength(11000) and .setMaxFieldLength(10000)

foundationdb-ci · 2022-07-22T19:00:41Z

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

Commit ID: 850a59b
Duration 4:17:36
Result: ❌ FAILED
Error: Build has timed out.
Build Logs (available for 30 days)

dlambrig · 2022-07-22T20:09:21Z

There is a bug in vv upgrade/downgrade, putting into draft until that is resolved

dlambrig · 2022-07-28T17:49:11Z

There is a bug in vv upgrade/downgrade, putting into draft until that is resolved

The problem was the new event occurred too many times and TracedTooManyLines was triggered, because the readVersion = commit Version is a common case and should not be logged.

fdb-windows-ci · 2022-07-28T20:34:11Z

Doxense CI Report for Windows 10

Commit ID: 42c23a8
Result: ✔️ SUCCEEDED
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-28T20:50:40Z

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Commit ID: 42c23a8
Duration 0:44:08
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-28T21:10:31Z

Result of foundationdb-pr on Linux CentOS 7

Commit ID: 42c23a8
Duration 1:03:57
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

foundationdb-ci · 2022-07-28T21:35:51Z

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

Commit ID: 42c23a8
Duration 1:29:19
Result: ✅ SUCCEEDED
Error: N/A
Build Logs (available for 30 days)

Track when version not found in version vector.

82624e6

dlambrig requested review from jzhou77 and sbodagala July 20, 2022 22:38

dlambrig commented Jul 20, 2022

View reviewed changes

jzhou77 requested changes Jul 21, 2022

View reviewed changes

Comment thread fdbclient/NativeAPI.actor.cpp Outdated

Comment thread fdbclient/NativeAPI.actor.cpp Outdated

Respond to comments

b01e8b8

dlambrig marked this pull request as ready for review July 21, 2022 14:50

dlambrig requested a review from jzhou77 July 21, 2022 14:50

dlambrig self-assigned this Jul 21, 2022

dlambrig added the diagnostics label Jul 21, 2022

sbodagala reviewed Jul 21, 2022

View reviewed changes

jzhou77 previously approved these changes Jul 22, 2022

View reviewed changes

sbodagala previously approved these changes Jul 22, 2022

View reviewed changes

include vv in event

850a59b

dlambrig dismissed stale reviews from sbodagala and jzhou77 via 850a59b July 22, 2022 14:42

sbodagala previously approved these changes Jul 22, 2022

View reviewed changes

jzhou77 reviewed Jul 22, 2022

View reviewed changes

jzhou77 previously approved these changes Jul 22, 2022

View reviewed changes

jzhou77 mentioned this pull request Jul 22, 2022

cherry pick 7653 - Track when version not found in version vector. #7666

Merged

5 tasks

dlambrig marked this pull request as draft July 22, 2022 20:08

Respond to review comments.

05558b5

dlambrig added 2 commits July 28, 2022 15:47

Merge remote-tracking branch 'origin/main' into clientCounter

05fde30

Fix bug where we logged too many lines

42c23a8

dlambrig dismissed stale reviews from jzhou77 and sbodagala via 42c23a8 July 28, 2022 20:06

dlambrig marked this pull request as ready for review July 28, 2022 21:19

dlambrig requested a review from jzhou77 July 28, 2022 21:20

jzhou77 approved these changes Jul 28, 2022

View reviewed changes

dlambrig merged commit 83d2351 into main Jul 28, 2022

dlambrig deleted the clientCounter branch July 28, 2022 22:26

Uh oh!

Conversation

dlambrig commented Jul 20, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdb-windows-ci commented Jul 20, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Jul 20, 2022

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

Uh oh!

Uh oh!

foundationdb-ci commented Jul 21, 2022

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

Uh oh!

fdb-windows-ci commented Jul 21, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Jul 21, 2022

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Uh oh!

foundationdb-ci commented Jul 21, 2022

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlambrig Jul 21, 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!

Choose a reason for hiding this comment

Uh oh!

foundationdb-ci commented Jul 21, 2022

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdb-windows-ci commented Jul 22, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Jul 22, 2022

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Uh oh!

foundationdb-ci commented Jul 22, 2022

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

foundationdb-ci commented Jul 22, 2022

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

Uh oh!

dlambrig commented Jul 22, 2022

Uh oh!

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

Uh oh!

Uh oh!

fdb-windows-ci commented Jul 28, 2022

Doxense CI Report for Windows 10

Uh oh!

foundationdb-ci commented Jul 28, 2022

Result of foundationdb-pr-macos on macOS BigSur 11.5.2

Uh oh!

foundationdb-ci commented Jul 28, 2022

Result of foundationdb-pr on Linux CentOS 7

Uh oh!

foundationdb-ci commented Jul 28, 2022

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

Uh oh!

Reviewers

dlambrig commented Jul 20, 2022 •

edited

Loading

dlambrig Jul 21, 2022 •

edited

Loading

dlambrig commented Jul 28, 2022 •

edited

Loading