Remove TestID from stateful configuration database classes, and fix some bugs

fdbclient/ConfigKnobs.cpp

@@ -43,6 +43,61 @@ ConfigKey ConfigKeyRef::decodeKey(KeyRef const& key) {
 	}
 }
 
+Value KnobValueRef::ToValueFunc::operator()(int v) const {
+	return BinaryWriter::toValue(v, Unversioned());
+}
+Value KnobValueRef::ToValueFunc::operator()(int64_t v) const {
+	return BinaryWriter::toValue(v, Unversioned());
+}
+Value KnobValueRef::ToValueFunc::operator()(bool v) const {
+	return BinaryWriter::toValue(v, Unversioned());
+}
+Value KnobValueRef::ToValueFunc::operator()(ValueRef v) const {
+	return v;
+}
+Value KnobValueRef::ToValueFunc::operator()(double v) const {
+	return BinaryWriter::toValue(v, Unversioned());
+}
+
+std::string KnobValueRef::ToStringFunc::operator()(int v) const {
+	return format("int:%d", v);
+}
+std::string KnobValueRef::ToStringFunc::operator()(int64_t v) const {
+	return format("int64_t:%ld", v);
+}
+std::string KnobValueRef::ToStringFunc::operator()(bool v) const {
+	return format("bool:%d", v);
+}
+std::string KnobValueRef::ToStringFunc::operator()(ValueRef v) const {
+	return "string:" + v.toString();
+}
+std::string KnobValueRef::ToStringFunc::operator()(double v) const {
+	return format("double:%lf", v);
+}
+
+KnobValue KnobValueRef::CreatorFunc::operator()(NoKnobFound) const {
+	ASSERT(false);
+	return {};
+}
+KnobValue KnobValueRef::CreatorFunc::operator()(int v) const {
+	return KnobValueRef(v);
+}
+KnobValue KnobValueRef::CreatorFunc::operator()(double v) const {
+	return KnobValueRef(v);
+}
+KnobValue KnobValueRef::CreatorFunc::operator()(int64_t v) const {
+	return KnobValueRef(v);
+}
+KnobValue KnobValueRef::CreatorFunc::operator()(bool v) const {
+	return KnobValueRef(v);
+}
+KnobValue KnobValueRef::CreatorFunc::operator()(std::string const& v) const {
+	return KnobValueRef(ValueRef(reinterpret_cast<uint8_t const*>(v.c_str()), v.size()));
+}
+KnobValue KnobValueRef::create(ParsedKnobValue const& v) {
+	return std::visit(CreatorFunc{}, v);
+}
+
 TEST_CASE("/fdbclient/ConfigDB/ConfigKey/EncodeDecode") {
 	Tuple tuple;
 	tuple << "class-A"_sr

fdbclient/ConfigKnobs.h

@@ -33,26 +33,28 @@ class KnobValueRef {
 	explicit KnobValueRef(Arena& arena, ValueRef const& v) : value(std::in_place_type<ValueRef>, arena, v) {}
 
 	struct CreatorFunc {
-		Standalone<KnobValueRef> operator()(NoKnobFound const&) const {
+		Standalone<KnobValueRef> operator()(NoKnobFound) const;
-			ASSERT(false);
+		Standalone<KnobValueRef> operator()(int) const;
-			return {};
+		Standalone<KnobValueRef> operator()(double) const;
-		}
+		Standalone<KnobValueRef> operator()(int64_t) const;
-		template <class T>
+		Standalone<KnobValueRef> operator()(bool) const;
-		Standalone<KnobValueRef> operator()(T const& v) const {
+		Standalone<KnobValueRef> operator()(std::string const& v) const;
-			return KnobValueRef(v);
+	};
-		}
+
-		Standalone<KnobValueRef> operator()(std::string const& v) const {
+	struct ToValueFunc {
-			Standalone<KnobValueRef> knobValue;
+		Value operator()(int) const;
-			return KnobValueRef(ValueRef(reinterpret_cast<uint8_t const*>(v.c_str()), v.size()));
+		Value operator()(int64_t) const;
-		}
+		Value operator()(bool) const;
+		Value operator()(ValueRef) const;
+		Value operator()(double) const;
 	};
 
 	struct ToStringFunc {
-		std::string operator()(int v) const { return format("%d", v); }
+		std::string operator()(int) const;
-		std::string operator()(int64_t v) const { return format("%ld", v); }
+		std::string operator()(int64_t) const;
-		std::string operator()(bool v) const { return format("%d", v); }
+		std::string operator()(bool) const;
-		std::string operator()(ValueRef v) const { return v.toString(); }
+		std::string operator()(ValueRef) const;
-		std::string operator()(double const& v) const { return format("%lf", v); }
+		std::string operator()(double) const;
 	};
 
 	template <class KnobsType>
@@ -72,6 +74,11 @@ class KnobValueRef {
 public:
 	static constexpr FileIdentifier file_identifier = 9297109;
 
+	template <class T>
+	static Value toValue(T const& v) {
+		return ToValueFunc{}(v);
+	}
+
 	KnobValueRef() = default;
 
 	explicit KnobValueRef(Arena& arena, KnobValueRef const& rhs) : value(rhs.value) {
@@ -80,7 +87,7 @@ public:
 		}
 	}
 
-	static Standalone<KnobValueRef> create(ParsedKnobValue const& v) { return std::visit(CreatorFunc{}, v); }
+	static Standalone<KnobValueRef> create(ParsedKnobValue const& v);
 
 	size_t expectedSize() const {
 		return std::holds_alternative<KeyRef>(value) ? std::get<KeyRef>(value).expectedSize() : 0;
@@ -98,10 +105,7 @@ public:
 
 	std::string toString() const { return std::visit(ToStringFunc{}, value); }
 
-	Value toValue() const {
+	Value toValue() const { return std::visit(ToValueFunc{}, value); }
-		auto s = toString();
-		return ValueRef(reinterpret_cast<uint8_t const*>(s.c_str()), s.size());
-	}
 
 	bool setKnob(std::string const& knobName, Knobs& knobs) const {
 		return std::visit(SetKnobFunc<Knobs>{ knobs, knobName }, value);

fdbclient/ServerKnobCollection.h

@@ -72,10 +72,15 @@ public:
 	}
 
 	void setKnob(std::string const& knobName, KnobValueRef const& knobValue) override {
-		// Assert because we should validate inputs beforehand
+		if (!knobValue.setKnob(knobName, clientKnobCollection.getMutableFlowKnobs()) &&
-		ASSERT(knobValue.setKnob(knobName, clientKnobCollection.getMutableFlowKnobs()) ||
+		    !knobValue.setKnob(knobName, clientKnobCollection.getMutableClientKnobs()) &&
-		       knobValue.setKnob(knobName, clientKnobCollection.getMutableClientKnobs()) ||
+		    !knobValue.setKnob(knobName, serverKnobs) && !knobValue.setKnob(knobName, testKnobs)) {
-		       knobValue.setKnob(knobName, serverKnobs) || knobValue.setKnob(knobName, testKnobs));
+			// Input should already have been validated, so this indicates a bug
+			TraceEvent(SevError, "FailedToSetKnob")
+			    .detail("KnobName", knobName)
+			    .detail("KnobValue", knobValue.toString());
+			ASSERT(false);
+		}
 	}
 };
 

fdbclient/SimpleConfigTransaction.actor.cpp

@@ -54,8 +54,8 @@ class SimpleConfigTransactionImpl {
 		if (!self->getVersionFuture.isValid()) {
 			self->getVersionFuture = getReadVersion(self);
 		}
+		state ConfigKey configKey = ConfigKey::decodeKey(key);
 		Version version = wait(self->getVersionFuture);
-		auto configKey = ConfigKey::decodeKey(key);
 		if (self->dID.present()) {
 			TraceEvent("SimpleConfigTransactionGettingValue", self->dID.get())
 			    .detail("ConfigClass", configKey.configClass)

fdbserver/ConfigBroadcaster.actor.cpp

@@ -139,12 +139,17 @@ class ConfigBroadcasterImpl {
 		for (const auto& versionedMutation : changes) {
 			if (versionedMutation.version > req.lastSeenVersion &&
 			    matchesConfigClass(req.configClassSet, versionedMutation.mutation.getConfigClass())) {
-				TraceEvent(SevDebug, "ConfigBroadcasterSendingChangeMutation", id)
+				TraceEvent te(SevDebug, "ConfigBroadcasterSendingChangeMutation", id);
-				    .detail("Version", versionedMutation.version)
+				te.detail("Version", versionedMutation.version)
 				    .detail("ReqLastSeenVersion", req.lastSeenVersion)
 				    .detail("ConfigClass", versionedMutation.mutation.getConfigClass())
-				    .detail("KnobName", versionedMutation.mutation.getKnobName())
+				    .detail("KnobName", versionedMutation.mutation.getKnobName());
-				    .detail("KnobValue", versionedMutation.mutation.getValue().toString());
+				if (versionedMutation.mutation.isSet()) {
+					te.detail("Op", "Set").detail("KnobValue", versionedMutation.mutation.getValue().toString());
+				} else {
+					te.detail("Op", "Clear");
+				}
+
 				reply.changes.push_back_deep(reply.changes.arena(), versionedMutation);
 			}
 		}
@@ -167,6 +172,9 @@ class ConfigBroadcasterImpl {
 						}
 					}
 					reply.version = impl->mostRecentVersion;
+					TraceEvent(SevDebug, "ConfigBroadcasterGotSnapshotRequest", impl->id)
+					    .detail("Size", reply.snapshot.size())
+					    .detail("Version", reply.version);
 					req.reply.send(reply);
 				}
 				when(ConfigBroadcastFollowerGetChangesRequest req = waitNext(cbfi.getChanges.getFuture())) {

fdbserver/ConfigDatabaseUnitTests.actor.cpp

@@ -56,7 +56,6 @@ Value longToValue(int64_t v) {
 }
 
 class WriteToTransactionEnvironment {
-	UID id;
 	std::string dataDir;
 	ConfigTransactionInterface cti;
 	ConfigFollowerInterface cfi;
@@ -94,8 +93,8 @@ class WriteToTransactionEnvironment {
 
 public:
 	WriteToTransactionEnvironment(std::string const& dataDir)
-	  : id(deterministicRandom()->randomUniqueID()), dataDir(dataDir),
+	  : dataDir(dataDir), node(IConfigDatabaseNode::createSimple(dataDir)) {
-	    node(IConfigDatabaseNode::createSimple(dataDir, id)) {
+		platform::eraseDirectoryRecursive(dataDir);
 		setup();
 	}
 
@@ -111,7 +110,7 @@ public:
 	void restartNode() {
 		cfiServer.cancel();
 		ctiServer.cancel();
-		node = IConfigDatabaseNode::createSimple(dataDir, id);
+		node = IConfigDatabaseNode::createSimple(dataDir);
 		setup();
 	}
 
@@ -131,10 +130,17 @@ class ReadFromLocalConfigEnvironment {
 
 	ACTOR static Future<Void> checkEventually(LocalConfiguration const* localConfiguration,
 	                                          Optional<int64_t> expected) {
+		state double lastMismatchTime = now();
 		loop {
 			if (localConfiguration->getTestKnobs().TEST_LONG == (expected.present() ? expected.get() : 0)) {
 				return Void();
 			}
+			if (now() > lastMismatchTime + 1.0) {
+				TraceEvent(SevWarn, "CheckEventuallyStillChecking")
+				    .detail("Expected", expected.present() ? expected.get() : 0)
+				    .detail("TestLong", localConfiguration->getTestKnobs().TEST_LONG);
+				lastMismatchTime = now();
+			}
 			wait(delayJittered(0.1));
 		}
 	}
@@ -151,13 +157,12 @@ public:
 	ReadFromLocalConfigEnvironment(std::string const& dataDir,
 	                               std::string const& configPath,
 	                               std::map<std::string, std::string> const& manualKnobOverrides)
-	  : id(deterministicRandom()->randomUniqueID()), dataDir(dataDir),
+	  : dataDir(dataDir), localConfiguration(dataDir, configPath, manualKnobOverrides, true), consumer(Never()) {}
-	    localConfiguration(dataDir, configPath, manualKnobOverrides, id), consumer(Never()) {}
 
 	Future<Void> setup() { return setup(this); }
 
 	Future<Void> restartLocalConfig(std::string const& newConfigPath) {
-		localConfiguration = LocalConfiguration(dataDir, newConfigPath, {}, id);
+		localConfiguration = LocalConfiguration(dataDir, newConfigPath, {}, true);
 		return setup();
 	}
 
@@ -274,7 +279,7 @@ class TransactionEnvironment {
 		state Key configKey = encodeConfigKey(configClass, "test_long"_sr);
 		state Optional<Value> value = wait(tr->get(configKey));
 		if (expected.present()) {
-			ASSERT(value.get() == longToValue(expected.get()));
+			ASSERT_EQ(BinaryReader::fromStringRef<int64_t>(value.get(), Unversioned()), expected.get());
 		} else {
 			ASSERT(!value.present());
 		}
@@ -329,7 +334,8 @@ public:
 	Future<Void> setup() { return Void(); }
 
 	void restartNode() { writeTo.restartNode(); }
-	Future<Void> set(Optional<KeyRef> configClass, int64_t value, KeyRef knobName = "test_long"_sr) {
+	template <class T>
+	Future<Void> set(Optional<KeyRef> configClass, T value, KeyRef knobName = "test_long"_sr) {
 		return writeTo.set(configClass, value, knobName);
 	}
 	Future<Void> clear(Optional<KeyRef> configClass) { return writeTo.clear(configClass); }
@@ -382,7 +388,10 @@ public:
 
 	Future<Void> compact() { return writeTo.compact(); }
 
-	Future<Void> set(Optional<KeyRef> configClass, int64_t value) { return writeTo.set(configClass, value); }
+	template <class T>
+	Future<Void> set(Optional<KeyRef> configClass, T const& value) {
+		return writeTo.set(configClass, value);
+	}
 	Future<Void> clear(Optional<KeyRef> configClass) { return writeTo.clear(configClass); }
 	Future<Void> check(Optional<int64_t> value) const { return readFrom.checkEventually(value); }
 	Future<Void> getError() const { return writeTo.getError() || readFrom.getError() || broadcastServer; }
@@ -424,50 +433,46 @@ Future<Void> compact(BroadcasterToLocalConfigEnvironment& env) {
 	return Void();
 }
 
-std::string getDataDir(UnitTestParameters const& params) {
-	return params.get("unitTestDataDir").get();
-}
-
 ACTOR template <class Env>
 Future<Void> testRestartLocalConfig(UnitTestParameters params) {
-	state Env env(getDataDir(params), "class-A");
+	state Env env(params.getDataDir(), "class-A");
 	wait(env.setup());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
-	wait(check(env, 1));
+	wait(check(env, int64_t{ 1 }));
 	wait(env.restartLocalConfig("class-A"));
-	wait(check(env, 1));
+	wait(check(env, int64_t{ 1 }));
 	wait(set(env, "class-A"_sr, 2));
-	wait(check(env, 2));
+	wait(check(env, int64_t{ 2 }));
 	return Void();
 }
 
 ACTOR template <class Env>
 Future<Void> testRestartLocalConfigAndChangeClass(UnitTestParameters params) {
-	state Env env(getDataDir(params), "class-A");
+	state Env env(params.getDataDir(), "class-A");
 	wait(env.setup());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
-	wait(check(env, 1));
+	wait(check(env, int64_t{ 1 }));
 	wait(env.restartLocalConfig("class-B"));
-	wait(check(env, 0));
+	wait(check(env, int64_t{ 0 }));
-	wait(set(env, "class-B"_sr, 2));
+	wait(set(env, "class-B"_sr, int64_t{ 2 }));
-	wait(check(env, 2));
+	wait(check(env, int64_t{ 2 }));
 	return Void();
 }
 
 ACTOR template <class Env>
 Future<Void> testSet(UnitTestParameters params) {
-	state LocalConfigEnvironment env(getDataDir(params), "class-A", {});
+	state Env env(params.getDataDir(), "class-A");
 	wait(env.setup());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
-	wait(check(env, 1));
+	wait(check(env, int64_t{ 1 }));
 	return Void();
 }
 
 ACTOR template <class Env>
 Future<Void> testClear(UnitTestParameters params) {
-	state LocalConfigEnvironment env(getDataDir(params), "class-A", {});
+	state Env env(params.getDataDir(), "class-A");
 	wait(env.setup());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
 	wait(clear(env, "class-A"_sr));
 	wait(check(env, Optional<int64_t>{}));
 	return Void();
@@ -475,48 +480,48 @@ Future<Void> testClear(UnitTestParameters params) {
 
 ACTOR template <class Env>
 Future<Void> testGlobalSet(UnitTestParameters params) {
-	state Env env(getDataDir(params), "class-A");
+	state Env env(params.getDataDir(), "class-A");
 	wait(env.setup());
-	wait(set(env, Optional<KeyRef>{}, 1));
+	wait(set(env, Optional<KeyRef>{}, int64_t{ 1 }));
-	env.check(1);
+	wait(check(env, int64_t{ 1 }));
-	wait(set(env, "class-A"_sr, 10));
+	wait(set(env, "class-A"_sr, int64_t{ 10 }));
-	env.check(10);
+	wait(check(env, int64_t{ 10 }));
 	return Void();
 }
 
 ACTOR template <class Env>
 Future<Void> testIgnore(UnitTestParameters params) {
-	state Env env(getDataDir(params), "class-A");
+	state Env env(params.getDataDir(), "class-A");
 	wait(env.setup());
-	wait(set(env, "class-B"_sr, 1));
+	wait(set(env, "class-B"_sr, int64_t{ 1 }));
 	choose {
 		when(wait(delay(5))) {}
-		when(wait(check(env, 1))) { ASSERT(false); }
+		when(wait(check(env, int64_t{ 1 }))) { ASSERT(false); }
 	}
 	return Void();
 }
 
 ACTOR template <class Env>
 Future<Void> testCompact(UnitTestParameters params) {
-	state Env env(getDataDir(params), "class-A");
+	state Env env(params.getDataDir(), "class-A");
 	wait(env.setup());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
 	wait(compact(env));
 	wait(check(env, 1));
-	wait(set(env, "class-A"_sr, 2));
+	wait(set(env, "class-A"_sr, int64_t{ 2 }));
 	wait(check(env, 2));
 	return Void();
 }
 
 ACTOR template <class Env>
 Future<Void> testChangeBroadcaster(UnitTestParameters params) {
-	state Env env(getDataDir(params), "class-A");
+	state Env env(params.getDataDir(), "class-A");
 	wait(env.setup());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
-	wait(check(env, 1));
+	wait(check(env, int64_t{ 1 }));
 	env.changeBroadcaster();
-	wait(set(env, "class-A"_sr, 2));
+	wait(set(env, "class-A"_sr, int64_t{ 2 }));
-	wait(check(env, 2));
+	wait(check(env, int64_t{ 2 }));
 	return Void();
 }
 
@@ -529,9 +534,9 @@ bool matches(Standalone<VectorRef<KeyRef>> const& vec, std::set<Key> const& comp
 }
 
 ACTOR Future<Void> testGetConfigClasses(UnitTestParameters params, bool doCompact) {
-	state TransactionEnvironment env(getDataDir(params));
+	state TransactionEnvironment env(params.getDataDir());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
-	wait(set(env, "class-B"_sr, 1));
+	wait(set(env, "class-B"_sr, int64_t{ 1 }));
 	if (doCompact) {
 		wait(compact(env));
 	}
@@ -541,18 +546,19 @@ ACTOR Future<Void> testGetConfigClasses(UnitTestParameters params, bool doCompac
 }
 
 ACTOR Future<Void> testGetKnobs(UnitTestParameters params, bool global, bool doCompact) {
-	state TransactionEnvironment env(getDataDir(params));
+	state TransactionEnvironment env(params.getDataDir());
 	state Optional<Key> configClass;
 	if (!global) {
 		configClass = "class-A"_sr;
 	}
-	wait(set(env, configClass.castTo<KeyRef>(), 1, "test_long"_sr));
+	wait(set(env, configClass.castTo<KeyRef>(), int64_t{ 1 }, "test_long"_sr));
-	wait(set(env, configClass.castTo<KeyRef>(), 1, "test_int"_sr));
+	wait(set(env, configClass.castTo<KeyRef>(), int{ 2 }, "test_int"_sr));
-	wait(set(env, "class-B"_sr, 1, "test_double"_sr)); // ignored
+	wait(set(env, "class-B"_sr, double{ 3.0 }, "test_double"_sr)); // ignored
 	if (doCompact) {
 		wait(compact(env));
 	}
-	Standalone<VectorRef<KeyRef>> knobNames = wait(env.getKnobNames(configClass.castTo<KeyRef>()));
+	Standalone<VectorRef<KeyRef>> knobNames =
+	    wait(waitOrError(env.getKnobNames(configClass.castTo<KeyRef>()), env.getError()));
 	ASSERT(matches(knobNames, { "test_long"_sr, "test_int"_sr }));
 	return Void();
 }
@@ -585,18 +591,18 @@ TEST_CASE("/fdbserver/ConfigDB/LocalConfiguration/GlobalSet") {
 }
 
 TEST_CASE("/fdbserver/ConfigDB/LocalConfiguration/ConflictingOverrides") {
-	state LocalConfigEnvironment env(getDataDir(params), "class-A/class-B", {});
+	state LocalConfigEnvironment env(params.getDataDir(), "class-A/class-B", {});
 	wait(env.setup());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
-	wait(set(env, "class-B"_sr, 10));
+	wait(set(env, "class-B"_sr, int64_t{ 10 }));
 	env.check(10);
 	return Void();
 }
 
 TEST_CASE("/fdbserver/ConfigDB/LocalConfiguration/Manual") {
-	state LocalConfigEnvironment env(getDataDir(params), "class-A", { { "test_long", "1000" } });
+	state LocalConfigEnvironment env(params.getDataDir(), "class-A", { { "test_long", "1000" } });
 	wait(env.setup());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
 	env.check(1000);
 	return Void();
 }
@@ -657,11 +663,11 @@ TEST_CASE("/fdbserver/ConfigDB/TransactionToLocalConfig/GlobalSet") {
 }
 
 TEST_CASE("/fdbserver/ConfigDB/TransactionToLocalConfig/RestartNode") {
-	state TransactionToLocalConfigEnvironment env(getDataDir(params), "class-A");
+	state TransactionToLocalConfigEnvironment env(params.getDataDir(), "class-A");
 	wait(env.setup());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
 	env.restartNode();
-	wait(check(env, 1));
+	wait(check(env, int64_t{ 1 }));
 	return Void();
 }
 
@@ -681,30 +687,37 @@ TEST_CASE("/fdbserver/ConfigDB/TransactionToLocalConfig/CompactNode") {
 }
 
 TEST_CASE("/fdbserver/ConfigDB/Transaction/Set") {
-	wait(testSet<TransactionEnvironment>(params));
+	state TransactionEnvironment env(params.getDataDir());
+	wait(env.setup());
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
+	wait(check(env, "class-A"_sr, int64_t{ 1 }));
 	return Void();
 }
 
 TEST_CASE("/fdbserver/ConfigDB/Transaction/Clear") {
-	wait(testClear<TransactionEnvironment>(params));
+	state TransactionEnvironment env(params.getDataDir());
+	wait(env.setup());
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
+	wait(clear(env, "class-A"_sr));
+	wait(check(env, "class-A"_sr, Optional<int64_t>{}));
 	return Void();
 }
 
 TEST_CASE("/fdbserver/ConfigDB/Transaction/Restart") {
-	state TransactionEnvironment env(getDataDir(params));
+	state TransactionEnvironment env(params.getDataDir());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
 	env.restartNode();
-	wait(check(env, "class-A"_sr, 1));
+	wait(check(env, "class-A"_sr, int64_t{ 1 }));
 	return Void();
 }
 
 TEST_CASE("/fdbserver/ConfigDB/Transaction/CompactNode") {
-	state TransactionEnvironment env(getDataDir(params));
+	state TransactionEnvironment env(params.getDataDir());
-	wait(set(env, "class-A"_sr, 1));
+	wait(set(env, "class-A"_sr, int64_t{ 1 }));
-	wait(env.compact());
+	wait(compact(env));
-	wait(env.check("class-A"_sr, 1));
+	wait(check(env, "class-A"_sr, int64_t{ 1 }));
-	wait(set(env, "class-A"_sr, 2));
+	wait(set(env, "class-A"_sr, int64_t{ 2 }));
-	wait(env.check("class-A"_sr, 2));
+	wait(check(env, "class-A"_sr, int64_t{ 2 }));
 	return Void();
 }
 
@@ -739,7 +752,7 @@ TEST_CASE("/fdbserver/ConfigDB/Transaction/CompactThenGetGlobalKnobs") {
 }
 
 TEST_CASE("/fdbserver/ConfigDB/Transaction/BadRangeRead") {
-	state TransactionEnvironment env(getDataDir(params));
+	state TransactionEnvironment env(params.getDataDir());
 	try {
 		wait(env.badRangeRead() || env.getError());
 		ASSERT(false);

fdbserver/Coordination.actor.cpp

@@ -135,8 +135,7 @@ ACTOR Future<Void> localGenerationReg(GenerationRegInterface interf, OnDemandSto
 
 TEST_CASE("/fdbserver/Coordination/localGenerationReg/simple") {
 	state GenerationRegInterface reg;
-	state OnDemandStore store(
+	state OnDemandStore store(params.getDataDir(), deterministicRandom()->randomUniqueID(), "coordination");
-	    params.get("unitTestDataDir").get(), deterministicRandom()->randomUniqueID(), "coordination");
 	state Future<Void> actor = localGenerationReg(reg, &store);
 	state Key the_key(deterministicRandom()->randomAlphaNumeric(deterministicRandom()->randomInt(0, 10)));
 

fdbserver/IConfigDatabaseNode.cpp

@@ -22,10 +22,10 @@
 #include "fdbserver/PaxosConfigDatabaseNode.h"
 #include "fdbserver/SimpleConfigDatabaseNode.h"
 
-Reference<IConfigDatabaseNode> IConfigDatabaseNode::createSimple(std::string const& folder, Optional<UID> testID) {
+Reference<IConfigDatabaseNode> IConfigDatabaseNode::createSimple(std::string const& folder) {
-	return makeReference<SimpleConfigDatabaseNode>(folder, testID);
+	return makeReference<SimpleConfigDatabaseNode>(folder);
 }
 
-Reference<IConfigDatabaseNode> IConfigDatabaseNode::createPaxos(std::string const& folder, Optional<UID> testID) {
+Reference<IConfigDatabaseNode> IConfigDatabaseNode::createPaxos(std::string const& folder) {
-	return makeReference<PaxosConfigDatabaseNode>(folder, testID);
+	return makeReference<PaxosConfigDatabaseNode>(folder);
 }

fdbserver/IConfigDatabaseNode.h

@@ -32,6 +32,6 @@ public:
 	virtual Future<Void> serve(ConfigTransactionInterface const&) = 0;
 	virtual Future<Void> serve(ConfigFollowerInterface const&) = 0;
 
-	static Reference<IConfigDatabaseNode> createSimple(std::string const& folder, Optional<UID> testID = {});
+	static Reference<IConfigDatabaseNode> createSimple(std::string const& folder);
-	static Reference<IConfigDatabaseNode> createPaxos(std::string const& folder, Optional<UID> testID = {});
+	static Reference<IConfigDatabaseNode> createPaxos(std::string const& folder);
 };

fdbserver/LocalConfiguration.actor.cpp

@@ -128,9 +128,15 @@ class LocalConfigurationImpl {
 	ConfigKnobOverrides configKnobOverrides;
 	std::unique_ptr<IKnobCollection> testKnobCollection;
 
-	IKnobCollection& getKnobs() { return testKnobCollection ? *testKnobCollection : *g_knobs; }
+	IKnobCollection& getKnobs() {
+		ASSERT(testKnobCollection);
+		return testKnobCollection ? *testKnobCollection : *g_knobs;
+	}
 
-	IKnobCollection const& getKnobs() const { return testKnobCollection ? *testKnobCollection : *g_knobs; }
+	IKnobCollection const& getKnobs() const {
+		ASSERT(testKnobCollection);
+		return testKnobCollection ? *testKnobCollection : *g_knobs;
+	}
 
 	CounterCollection cc;
 	Counter broadcasterChanges;
@@ -289,13 +295,12 @@ public:
 	LocalConfigurationImpl(std::string const& dataFolder,
 	                       std::string const& configPath,
 	                       std::map<std::string, std::string> const& manualKnobOverrides,
-	                       Optional<UID> testID)
+	                       bool isTest)
-	  : id(testID.present() ? testID.get() : deterministicRandom()->randomUniqueID()),
+	  : id(deterministicRandom()->randomUniqueID()), kvStore(dataFolder, id, "localconf-"), cc("LocalConfiguration"),
-	    kvStore(dataFolder, id, "localconf-" + (testID.present() ? id.toString() : "")), cc("LocalConfiguration"),
 	    broadcasterChanges("BroadcasterChanges", cc), snapshots("Snapshots", cc),
 	    changeRequestsFetched("ChangeRequestsFetched", cc), mutations("Mutations", cc), configKnobOverrides(configPath),
 	    manualKnobOverrides(manualKnobOverrides) {
-		if (testID.present()) {
+		if (isTest) {
 			testKnobCollection = IKnobCollection::createServerKnobCollection(true, g_network->isSimulated());
 		}
 		logger = traceCounters(
@@ -342,8 +347,8 @@ public:
 LocalConfiguration::LocalConfiguration(std::string const& dataFolder,
                                        std::string const& configPath,
                                        std::map<std::string, std::string> const& manualKnobOverrides,
-                                       Optional<UID> testID)
+                                       bool isTest)
-  : impl(std::make_unique<LocalConfigurationImpl>(dataFolder, configPath, manualKnobOverrides, testID)) {}
+  : impl(std::make_unique<LocalConfigurationImpl>(dataFolder, configPath, manualKnobOverrides, isTest)) {}
 
 LocalConfiguration::LocalConfiguration(LocalConfiguration&&) = default;
 

fdbserver/LocalConfiguration.h

@@ -36,7 +36,7 @@ public:
 	LocalConfiguration(std::string const& dataFolder,
 	                   std::string const& configPath,
 	                   std::map<std::string, std::string> const& manualKnobOverrides,
-	                   Optional<UID> testID = {});
+	                   bool isTest = false);
 	LocalConfiguration(LocalConfiguration&&);
 	LocalConfiguration& operator=(LocalConfiguration&&);
 	~LocalConfiguration();

fdbserver/OnDemandStore.actor.cpp

@@ -50,8 +50,8 @@ IKeyValueStore* OnDemandStore::get() {
 }
 
 bool OnDemandStore::exists() const {
-	return store || fileExists(joinPath(folder, prefix + "-0.fdq")) ||
+	return store || fileExists(joinPath(folder, prefix + "0.fdq")) || fileExists(joinPath(folder, prefix + "1.fdq")) ||
-	       fileExists(joinPath(folder, prefix + "-1.fdq")) || fileExists(joinPath(folder, prefix + ".fdb"));
+	       fileExists(joinPath(folder, prefix + ".fdb"));
 }
 
 IKeyValueStore* OnDemandStore::operator->() {

fdbserver/PaxosConfigDatabaseNode.actor.cpp

@@ -22,7 +22,7 @@
 
 class PaxosConfigDatabaseNodeImpl {};
 
-PaxosConfigDatabaseNode::PaxosConfigDatabaseNode(std::string const& folder, Optional<UID> testID) {
+PaxosConfigDatabaseNode::PaxosConfigDatabaseNode(std::string const& folder) {
 	// TODO: Implement
 	ASSERT(false);
 }

fdbserver/PaxosConfigDatabaseNode.h

@@ -26,7 +26,7 @@ class PaxosConfigDatabaseNode : public IConfigDatabaseNode {
 	std::unique_ptr<class PaxosConfigDatabaseNodeImpl> impl;
 
 public:
-	PaxosConfigDatabaseNode(std::string const& folder, Optional<UID> testID = {});
+	PaxosConfigDatabaseNode(std::string const& folder);
 	~PaxosConfigDatabaseNode();
 	Future<Void> serve(ConfigTransactionInterface const&) override;
 	Future<Void> serve(ConfigFollowerInterface const&) override;

fdbserver/SimpleConfigConsumer.actor.cpp

@@ -57,11 +57,15 @@ class SimpleConfigConsumerImpl {
 				    wait(self->cfi.getChanges.getReply(ConfigFollowerGetChangesRequest{ self->lastSeenVersion }));
 				++self->successfulChangeRequest;
 				for (const auto& versionedMutation : reply.changes) {
-					TraceEvent(SevDebug, "ConsumerFetchedMutation", self->id)
+					TraceEvent te(SevDebug, "ConsumerFetchedMutation", self->id);
-					    .detail("Version", versionedMutation.version)
+					te.detail("Version", versionedMutation.version)
 					    .detail("ConfigClass", versionedMutation.mutation.getConfigClass())
-					    .detail("KnobName", versionedMutation.mutation.getKnobName())
+					    .detail("KnobName", versionedMutation.mutation.getKnobName());
-					    .detail("KnobValue", versionedMutation.mutation.getValue().toString());
+					if (versionedMutation.mutation.isSet()) {
+						te.detail("Op", "Set").detail("KnobValue", versionedMutation.mutation.getValue().toString());
+					} else {
+						te.detail("Op", "Clear");
+					}
 				}
 				if (reply.mostRecentVersion > self->lastSeenVersion) {
 					self->lastSeenVersion = reply.mostRecentVersion;

fdbserver/SimpleConfigDatabaseNode.actor.cpp

@@ -20,6 +20,7 @@
 
 #include <map>
 
+#include "fdbclient/SystemData.h"
 #include "fdbserver/SimpleConfigDatabaseNode.h"
 #include "fdbserver/IKeyValueStore.h"
 #include "fdbserver/OnDemandStore.h"
@@ -230,7 +231,11 @@ class SimpleConfigDatabaseNodeImpl {
 		for (const auto &versionedMutation : versionedMutations) {
 			const auto &mutation = versionedMutation.mutation;
 			if (mutation.getKey() == req.key) {
-				value = mutation.getValue();
+				if (mutation.isSet()) {
+					value = mutation.getValue();
+				} else {
+					value = {};
+				}
 			}
 		}
 		req.reply.send(ConfigTransactionGetReply{ value });
@@ -314,6 +319,11 @@ class SimpleConfigDatabaseNodeImpl {
 			Key key = versionedMutationKey(req.version, index++);
 			Value value = ObjectWriter::toValue(mutation, IncludeVersion());
 			if (mutation.isSet()) {
+				TraceEvent("SimpleConfigDatabaseNodeSetting")
+				    .detail("ConfigClass", mutation.getConfigClass())
+				    .detail("KnobName", mutation.getKnobName())
+				    .detail("Value", mutation.getValue().toString())
+				    .detail("Version", req.version);
 				++self->setMutations;
 			} else {
 				++self->clearMutations;
@@ -367,7 +377,7 @@ class SimpleConfigDatabaseNodeImpl {
 		wait(store(reply.changesVersion, getCommittedVersion(self)));
 		wait(store(reply.changes, getMutations(self, reply.snapshotVersion + 1, reply.changesVersion)));
 		wait(store(reply.annotations, getAnnotations(self, reply.snapshotVersion + 1, reply.changesVersion)));
-		TraceEvent(SevDebug, "ConfigDatabaseNodeGettingSnapshot")
+		TraceEvent(SevDebug, "ConfigDatabaseNodeGettingSnapshot", self->id)
 		    .detail("SnapshotVersion", reply.snapshotVersion)
 		    .detail("ChangesVersion", reply.changesVersion)
 		    .detail("SnapshotSize", reply.snapshot.size())
@@ -379,7 +389,7 @@ class SimpleConfigDatabaseNodeImpl {
 
 	ACTOR static Future<Void> compact(SimpleConfigDatabaseNodeImpl* self, ConfigFollowerCompactRequest req) {
 		state Version lastCompactedVersion = wait(getLastCompactedVersion(self));
-		TraceEvent(SevDebug, "ConfigDatabaseNodeCompacting")
+		TraceEvent(SevDebug, "ConfigDatabaseNodeCompacting", self->id)
 		    .detail("Version", req.version)
 		    .detail("LastCompacted", lastCompactedVersion);
 		if (req.version <= lastCompactedVersion) {
@@ -441,9 +451,8 @@ class SimpleConfigDatabaseNodeImpl {
 	}
 
 public:
-	SimpleConfigDatabaseNodeImpl(std::string const& folder, Optional<UID> testID)
+	SimpleConfigDatabaseNodeImpl(std::string const& folder)
-	  : id(testID.present() ? testID.get() : deterministicRandom()->randomUniqueID()),
+	  : id(deterministicRandom()->randomUniqueID()), kvStore(folder, id, "globalconf-"), cc("ConfigDatabaseNode"),
-	    kvStore(folder, id, "globalconf-" + (testID.present() ? id.toString() : "")), cc("ConfigDatabaseNode"),
 	    compactRequests("CompactRequests", cc), successfulChangeRequests("SuccessfulChangeRequests", cc),
 	    failedChangeRequests("FailedChangeRequests", cc), snapshotRequests("SnapshotRequests", cc),
 	    successfulCommits("SuccessfulCommits", cc), failedCommits("FailedCommits", cc),
@@ -451,6 +460,7 @@ public:
 	    getValueRequests("GetValueRequests", cc), newVersionRequests("NewVersionRequests", cc) {
 		logger = traceCounters(
 		    "ConfigDatabaseNodeMetrics", id, SERVER_KNOBS->WORKER_LOGGING_INTERVAL, &cc, "ConfigDatabaseNode");
+		TraceEvent(SevDebug, "StartingSimpleConfigDatabaseNode", id).detail("KVStoreAlreadyExists", kvStore.exists());
 	}
 
 	Future<Void> serve(ConfigTransactionInterface const& cti) { return serve(this, &cti); }
@@ -458,8 +468,8 @@ public:
 	Future<Void> serve(ConfigFollowerInterface const& cfi) { return serve(this, &cfi); }
 };
 
-SimpleConfigDatabaseNode::SimpleConfigDatabaseNode(std::string const& folder, Optional<UID> testID)
+SimpleConfigDatabaseNode::SimpleConfigDatabaseNode(std::string const& folder)
-  : impl(std::make_unique<SimpleConfigDatabaseNodeImpl>(folder, testID)) {}
+  : impl(std::make_unique<SimpleConfigDatabaseNodeImpl>(folder)) {}
 
 SimpleConfigDatabaseNode::~SimpleConfigDatabaseNode() = default;
 

fdbserver/SimpleConfigDatabaseNode.h

@@ -26,7 +26,7 @@ class SimpleConfigDatabaseNode : public IConfigDatabaseNode {
 	std::unique_ptr<class SimpleConfigDatabaseNodeImpl> impl;
 
 public:
-	SimpleConfigDatabaseNode(std::string const& folder, Optional<UID> testID = {});
+	SimpleConfigDatabaseNode(std::string const& folder);
 	~SimpleConfigDatabaseNode();
 	Future<Void> serve(ConfigTransactionInterface const&) override;
 	Future<Void> serve(ConfigFollowerInterface const&) override;

fdbserver/workloads/UnitTests.actor.cpp

@@ -35,7 +35,6 @@ struct UnitTestWorkload : TestWorkload {
 	std::string testPattern;
 	int testRunLimit;
 	UnitTestParameters testParams;
-	std::string dataDir;
 
 	PerfIntCounter testsAvailable, testsExecuted, testsFailed;
 	PerfDoubleCounter totalWallTime, totalSimTime;
@@ -47,8 +46,7 @@ struct UnitTestWorkload : TestWorkload {
 		enabled = !clientId; // only do this on the "first" client
 		testPattern = getOption(options, LiteralStringRef("testsMatching"), Value()).toString();
 		testRunLimit = getOption(options, LiteralStringRef("maxTestCases"), -1);
-		dataDir = getOption(options, LiteralStringRef("unitTestDataDir"), "simfdb/unittests/"_sr).toString();
+		testParams.setDataDir(getOption(options, LiteralStringRef("dataDir"), "simfdb/unittests/"_sr).toString());
-		testParams.set("unitTestDataDir", dataDir);
 
 		// Consume all remaining options as testParams which the unit test can access
 		for (auto& kv : options) {
@@ -67,7 +65,10 @@ struct UnitTestWorkload : TestWorkload {
 	}
 
 	std::string description() const override { return "UnitTests"; }
-	Future<Void> setup(Database const& cx) override { return Void(); }
+	Future<Void> setup(Database const& cx) override {
+		platform::eraseDirectoryRecursive(testParams.getDataDir());
+		return Void();
+	}
 	Future<Void> start(Database const& cx) override {
 		if (enabled)
 			return runUnitTests(this);
@@ -105,14 +106,14 @@ struct UnitTestWorkload : TestWorkload {
 			state double start_now = now();
 			state double start_timer = timer();
 
+			platform::createDirectory(self->testParams.getDataDir());
 			try {
-				platform::createDirectory(self->dataDir);
 				wait(test->func(self->testParams));
 			} catch (Error& e) {
 				++self->testsFailed;
 				result = e;
 			}
-			platform::eraseDirectoryRecursive(self->dataDir);
+			platform::eraseDirectoryRecursive(self->testParams.getDataDir());
 			++self->testsExecuted;
 			double wallTime = timer() - start_timer;
 			double simTime = now() - start_now;

flow/Knobs.cpp

@@ -183,7 +183,8 @@ void FlowKnobs::initialize(bool randomize, bool isSimulated) {
 	init( ZERO_LENGTH_FILE_PAD,                                  1 );
 	init( TRACE_FLUSH_INTERVAL,                               0.25 );
 	init( TRACE_RETRY_OPEN_INTERVAL,						  1.00 );
-	init( MIN_TRACE_SEVERITY,                 isSimulated ? 1 : 10 ); // Related to the trace severity in Trace.h
+	// FIXME: Undo
+	init( MIN_TRACE_SEVERITY,                  isSimulated ? 1 : 5 ); // Related to the trace severity in Trace.h
 	init( MAX_TRACE_SUPPRESSIONS,                              1e4 );
 	init( TRACE_DATETIME_ENABLED,                             true ); // trace time in human readable format (always real time)
 	init( TRACE_SYNC_ENABLED,                                    0 );
@@ -254,8 +255,9 @@ static std::string toLower(std::string const& name) {
 	return lower_name;
 }
 
-static int64_t parseInt64(std::string const& value) {
+template <class T>
-	int64_t v;
+static T parseIntegral(std::string const& value) {
+	T v;
 	int n = 0;
 	if (StringRef(value).startsWith(LiteralStringRef("0x"))) {
 		if (sscanf(value.c_str(), "0x%" SCNx64 "%n", &v, &n) != 1 || n != value.size())
@@ -280,12 +282,12 @@ ParsedKnobValue Knobs::parseKnobValue(std::string const& knob, std::string const
 		} else if (toLower(value) == "false") {
 			return false;
 		} else {
-			return parseInt64(value);
+			return parseIntegral<bool>(value);
 		}
 	} else if (int64_knobs.count(knob)) {
-		return parseInt64(value);
+		return parseIntegral<int64_t>(value);
 	} else if (int_knobs.count(knob)) {
-		return parseInt64(value);
+		return parseIntegral<int>(value);
 	} else if (string_knobs.count(knob)) {
 		return value;
 	}

flow/UnitTest.cpp

@@ -63,3 +63,11 @@ Optional<double> UnitTestParameters::getDouble(const std::string& name) const {
 	}
 	return {};
 }
+
+std::string UnitTestParameters::getDataDir() const {
+	return dataDir.get();
+}
+
+void UnitTestParameters::setDataDir(std::string const& dataDir) {
+	this->dataDir = dataDir;
+}

flow/UnitTest.h

@@ -47,7 +47,10 @@
 
 #include <cinttypes>
 
-struct UnitTestParameters {
+class UnitTestParameters {
+	Optional<std::string> dataDir;
+
+public:
 	// Map of named case-sensitive parameters
 	std::map<std::string, std::string> params;
 
@@ -68,6 +71,12 @@ struct UnitTestParameters {
 
 	// Get a parameter's value parsed as a double, will return !present() if parameter was not set
 	Optional<double> getDouble(const std::string& name) const;
+
+	// This is separate because it assumes data directory has already been set, and doesn't return an optional
+	std::string getDataDir() const;
+
+	// Set the data directory used to persist data for this test
+	void setDataDir(std::string const&);
 };
 
 // Unit test definition structured as a linked list item

tests/SpecificUnitTest.txt

@@ -3,5 +3,5 @@ startDelay=0
 useDB=false
 
     testName=UnitTests
-    maxTestCases=100
+    maxTestCases=1
     testsMatching=/fdbserver/ConfigDB/