From 2e63ac6963fed1a66a0924e64f544290cf4a3d87 Mon Sep 17 00:00:00 2001
From: Jon Fu <jon.fu@snowflake.com>
Date: Thu, 10 Feb 2022 17:01:09 -0500
Subject: [PATCH] code review changes: additional test checks, cleanup extra
code, mark internal option hidden
---
fdbclient/CommitProxyInterface.h | 2 +-
fdbclient/NativeAPI.actor.cpp | 6 ++++--
fdbclient/ReadYourWrites.h | 2 --
fdbclient/vexillographer/fdb.options | 3 ++-
fdbserver/workloads/SidebandSingle.actor.cpp | 10 +++++++++-
5 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/fdbclient/CommitProxyInterface.h b/fdbclient/CommitProxyInterface.h
index a0d396694c..7e670ed08a 100644
--- a/fdbclient/CommitProxyInterface.h
+++ b/fdbclient/CommitProxyInterface.h
@@ -203,7 +203,7 @@ struct GetReadVersionReply : public BasicLoadBalancedReply {
TransactionTagMap<ClientTagThrottleLimits> tagThrottleInfo;
- GetReadVersionReply() : version(invalidVersion), locked(false), rkThrottled(false) {}
+ GetReadVersionReply() : version(invalidVersion), locked(false) {}
template <class Ar>
void serialize(Ar& ar) {
diff --git a/fdbclient/NativeAPI.actor.cpp b/fdbclient/NativeAPI.actor.cpp
index 4c8947d81f..4d05bf2974 100644
--- a/fdbclient/NativeAPI.actor.cpp
+++ b/fdbclient/NativeAPI.actor.cpp
@@ -1066,8 +1066,10 @@ ACTOR static Future<Void> backgroundGrvUpdater(DatabaseContext* cx) {
wait(tr.onError(e));
}
} else {
- wait(delay(0.001 + std::min(CLIENT_KNOBS->MAX_PROXY_CONTACT_LAG - (curTime - lastProxyTime),
- (CLIENT_KNOBS->MAX_VERSION_CACHE_LAG - grvDelay) - (curTime - lastTime))));
+ wait(
+ delay(std::max(0.001,
+ std::min(CLIENT_KNOBS->MAX_PROXY_CONTACT_LAG - (curTime - lastProxyTime),
+ (CLIENT_KNOBS->MAX_VERSION_CACHE_LAG - grvDelay) - (curTime - lastTime)))));
}
}
} catch (Error& e) {
diff --git a/fdbclient/ReadYourWrites.h b/fdbclient/ReadYourWrites.h
index 147cc75499..b8ccd23e54 100644
--- a/fdbclient/ReadYourWrites.h
+++ b/fdbclient/ReadYourWrites.h
@@ -44,8 +44,6 @@ struct ReadYourWritesTransactionOptions {
int maxRetries;
int snapshotRywEnabled;
bool bypassUnreadable : 1;
- bool useGrvCache : 1;
- bool skipGrvCache : 1;
ReadYourWritesTransactionOptions() {}
explicit ReadYourWritesTransactionOptions(Transaction const& tr);
diff --git a/fdbclient/vexillographer/fdb.options b/fdbclient/vexillographer/fdb.options
index 3e91511bf7..c4845ae8bd 100644
--- a/fdbclient/vexillographer/fdb.options
+++ b/fdbclient/vexillographer/fdb.options
@@ -295,7 +295,8 @@ description is not currently required but encouraged.
<Option name="use_grv_cache" code="1101"
description="Allows this transaction to use cached GRV from the database context. Defaults to off. Upon first usage, starts a background updater to periodically update the cache to avoid stale read versions." />
<Option name="skip_grv_cache" code="1102"
- description="Specifically instruct this transaction to NOT use cached GRV. Primarily used for the read version cache's background updater to avoid attempting to read a cached entry in specific situations." />
+ description="Specifically instruct this transaction to NOT use cached GRV. Primarily used for the read version cache's background updater to avoid attempting to read a cached entry in specific situations."
+ hidden="true"/>
</Scope>
<!-- The enumeration values matter - do not change them without
diff --git a/fdbserver/workloads/SidebandSingle.actor.cpp b/fdbserver/workloads/SidebandSingle.actor.cpp
index aad4b07738..0ec80a9ef9 100644
--- a/fdbserver/workloads/SidebandSingle.actor.cpp
+++ b/fdbserver/workloads/SidebandSingle.actor.cpp
@@ -217,6 +217,14 @@ struct SidebandSingleWorkload : TestWorkload {
tr.getReadVersion().get()); // will assert that ReadVersion is set
++self->consistencyErrors;
} else if (val.get() != LiteralStringRef("deadbeef")) {
+ // If we read something NOT "deadbeef" and there was no commit_unknown_result,
+ // the cache somehow read a stale version of our key
+ if (message.commitVersion != invalidVersion) {
+ TraceEvent(SevError, "CausalConsistencyError2", self->interf.id())
+ .detail("MessageKey", messageKey.toString().c_str());
+ ++self->consistencyErrors;
+ break;
+ }
// check again without cache, and if it's the same, that's expected
state Transaction tr2(cx);
state Optional<Value> val2;
@@ -230,7 +238,7 @@ struct SidebandSingleWorkload : TestWorkload {
}
}
if (val != val2) {
- TraceEvent(SevError, "CausalConsistencyError2", self->interf.id())
+ TraceEvent(SevError, "CausalConsistencyError3", self->interf.id())
.detail("MessageKey", messageKey.toString().c_str())
.detail("Val1", val)
.detail("Val2", val2)