Follow-up T180252295: Allow same initial and max backoff for kvstore …

…peer sync

Summary:
In a follow-up to T180252295. We want to allow kvstore peer sync backoffs configurable to same initial and max backoff value. Existing code had an assert at places and does not allow. Changes in this diffs,
- Remove assert checking equality of 2 values
- Relax configuration parameter check to allow max backoff to be same value as initial backoff
- Add Exponential backoff API unit tests
- Add kvstore peer sync unit tests

Not covered in this diffs: moving to friend class for use of processThriftSuccess/processThriftFailure APIs from test code.

Reviewed By: xiangxu1121

Differential Revision: D55074731

fbshipit-source-id: 2e7312c2d12f0988115d0a3fd9ec18860688946a

openr/common/ExponentialBackoff.cpp

@@ -23,8 +23,8 @@ ExponentialBackoff<Duration>::ExponentialBackoff(
       isAbortAtMax_(isAbortAtMax) {
   XCHECK_GT(initialBackoff.count(), Duration(0).count())
       << "Backoff must be positive value";
-  XCHECK_LT(initialBackoff.count(), maxBackoff.count())
-      << "Max backoff must be greater than initial backoff.";
+  XCHECK_LE(initialBackoff.count(), maxBackoff.count())
+      << "Max backoff must be greater than or equal to initial backoff.";
 }
 
 template <typename Duration>

openr/common/tests/ExponentialBackoffTest.cpp

@@ -88,6 +88,41 @@ TEST(ExponentialBackoffTest, ApiTest) {
   EXPECT_EQ(timer.getTimeRemainingUntilRetry(), std::chrono::milliseconds(0));
 }
 
+TEST(ExponentialBackoffTest, SameInitialAndMaxBackoffTest) {
+  openr::ExponentialBackoff<std::chrono::milliseconds> timer(
+      std::chrono::milliseconds(2), std::chrono::milliseconds(2));
+  EXPECT_TRUE(timer.canTryNow());
+  EXPECT_FALSE(timer.atMaxBackoff());
+  EXPECT_EQ(timer.getTimeRemainingUntilRetry(), std::chrono::milliseconds(0));
+
+  timer.reportSuccess();
+  EXPECT_TRUE(timer.canTryNow());
+  EXPECT_FALSE(timer.atMaxBackoff());
+  EXPECT_EQ(timer.getTimeRemainingUntilRetry(), std::chrono::milliseconds(0));
+
+  // Report error and timer should become 2
+  timer.reportError();
+  EXPECT_FALSE(timer.canTryNow());
+  EXPECT_TRUE(timer.atMaxBackoff());
+  EXPECT_GE(timer.getTimeRemainingUntilRetry(), std::chrono::milliseconds(1));
+  EXPECT_LE(timer.getTimeRemainingUntilRetry(), std::chrono::milliseconds(2));
+
+  // Report success and things should fall back to normal
+  timer.reportSuccess();
+  EXPECT_TRUE(timer.canTryNow());
+  EXPECT_FALSE(timer.atMaxBackoff());
+  EXPECT_EQ(timer.getTimeRemainingUntilRetry(), std::chrono::milliseconds(0));
+
+  // Increase wait timer
+  timer.reportError(); // 2ms
+  timer.reportError(); // 2ms
+  timer.reportError(); // 2ms
+  EXPECT_FALSE(timer.canTryNow());
+  EXPECT_TRUE(timer.atMaxBackoff());
+  EXPECT_GE(timer.getTimeRemainingUntilRetry(), std::chrono::milliseconds(1));
+  EXPECT_LE(timer.getTimeRemainingUntilRetry(), std::chrono::milliseconds(2));
+}
+
 int
 main(int argc, char** argv) {
   // Basic initialization

openr/kvstore/KvStore-inl.h

@@ -58,7 +58,7 @@ KvStore<ClientType>::KvStore(
         std::chrono::milliseconds(*kvStoreConfig.sync_initial_backoff_ms());
   }
 
-  if (*kvStoreConfig.sync_max_backoff_ms() <=
+  if (*kvStoreConfig.sync_max_backoff_ms() <
       std::chrono::milliseconds(kvParams_.syncInitialBackoff).count()) {
     if (kvParams_.syncInitialBackoff < Constants::kKvstoreSyncMaxBackoff) {
       kvParams_.syncMaxBackoff = Constants::kKvstoreSyncMaxBackoff;

openr/kvstore/tests/KvStoreTest.cpp

@@ -758,6 +758,84 @@ TEST_F(KvStoreTestFixture, PeerResyncWithConfiguredBackoff) {
   EXPECT_EQ(*cmpPeers["storeB"].state(), thrift::KvStorePeerState::INITIALIZED);
 }
 
+TEST_F(KvStoreTestFixture, PeerResyncWithEqualConfiguredBackoff) {
+  const std::chrono::milliseconds ksyncInitialBackoff(1000);
+  const std::chrono::milliseconds ksyncMaxBackoff(1000);
+  const std::chrono::milliseconds ksyncValidationTime(2000);
+
+  auto config = getTestKvConf("storeA");
+  config.sync_initial_backoff_ms() = ksyncInitialBackoff.count();
+  config.sync_max_backoff_ms() = ksyncMaxBackoff.count();
+
+  auto* storeA = createKvStore(config, {kTestingAreaName});
+  auto* storeB = createKvStore(getTestKvConf("storeB"));
+  auto* storeC = createKvStore(getTestKvConf("storeC"));
+  storeA->run();
+  storeB->run();
+  storeC->run();
+
+  // A - B
+  EXPECT_TRUE(storeB->addPeer(
+      kTestingAreaName, storeA->getNodeId(), storeA->getPeerSpec()));
+  EXPECT_TRUE(storeA->addPeer(
+      kTestingAreaName, storeB->getNodeId(), storeB->getPeerSpec()));
+
+  // B - C
+  EXPECT_TRUE(storeB->addPeer(
+      kTestingAreaName, storeC->getNodeId(), storeC->getPeerSpec()));
+  EXPECT_TRUE(storeC->addPeer(
+      kTestingAreaName, storeB->getNodeId(), storeB->getPeerSpec()));
+
+  waitForAllPeersInitialized();
+  auto cmpPeers = storeA->getPeers(kTestingAreaName);
+  EXPECT_EQ(1, cmpPeers.size());
+  EXPECT_EQ(*cmpPeers["storeB"].state(), thrift::KvStorePeerState::INITIALIZED);
+
+  storeA->injectThriftFailure(kTestingAreaName, "storeB");
+  auto start = std::chrono::steady_clock::now();
+
+  cmpPeers = storeA->getPeers(kTestingAreaName);
+  EXPECT_EQ(1, cmpPeers.size());
+  EXPECT_EQ(*cmpPeers["storeB"].state(), thrift::KvStorePeerState::IDLE);
+
+  waitForAllPeersInitialized();
+  auto elapsedTime =
+      duration_cast<milliseconds>(steady_clock::now() - start).count();
+  EXPECT_GT(elapsedTime, ksyncInitialBackoff.count());
+  EXPECT_LT(elapsedTime, ksyncValidationTime.count());
+
+  cmpPeers = storeA->getPeers(kTestingAreaName);
+  EXPECT_EQ(1, cmpPeers.size());
+  EXPECT_EQ(*cmpPeers["storeB"].state(), thrift::KvStorePeerState::INITIALIZED);
+
+  start = steady_clock::now();
+  storeA->injectThriftFailure(kTestingAreaName, "storeB");
+
+  OpenrEventBase evb;
+  int scheduleAt{0};
+  evb.scheduleTimeout(
+      std::chrono::milliseconds(
+          scheduleAt += (ksyncInitialBackoff.count() / 2)),
+      [&]() noexcept {
+        storeA->injectThriftFailure(kTestingAreaName, "storeB");
+        evb.stop();
+      });
+
+  // Start the event loop and wait until it is finished execution.
+  evb.run();
+  evb.waitUntilStopped();
+
+  waitForAllPeersInitialized();
+  elapsedTime =
+      duration_cast<milliseconds>(steady_clock::now() - start).count();
+  EXPECT_GT(elapsedTime, ksyncMaxBackoff.count());
+  EXPECT_LT(elapsedTime, ksyncValidationTime.count());
+
+  cmpPeers = storeA->getPeers(kTestingAreaName);
+  EXPECT_EQ(1, cmpPeers.size());
+  EXPECT_EQ(*cmpPeers["storeB"].state(), thrift::KvStorePeerState::INITIALIZED);
+}
+
 TEST_F(KvStoreTestFixture, PeerResyncWithDefaultBackoff) {
   auto* storeA = createKvStore(getTestKvConf("storeA"));
   auto* storeB = createKvStore(getTestKvConf("storeB"));
@@ -783,8 +861,8 @@ TEST_F(KvStoreTestFixture, PeerResyncWithDefaultBackoff) {
   EXPECT_EQ(1, cmpPeers.size());
   EXPECT_EQ(*cmpPeers["storeB"].state(), thrift::KvStorePeerState::INITIALIZED);
 
-  storeA->injectThriftFailure(kTestingAreaName, "storeB");
   auto start = std::chrono::steady_clock::now();
+  storeA->injectThriftFailure(kTestingAreaName, "storeB");
 
   cmpPeers = storeA->getPeers(kTestingAreaName);
   EXPECT_EQ(1, cmpPeers.size());
@@ -793,7 +871,9 @@ TEST_F(KvStoreTestFixture, PeerResyncWithDefaultBackoff) {
   waitForAllPeersInitialized();
   auto elapsedTime =
       duration_cast<milliseconds>(steady_clock::now() - start).count();
-  EXPECT_GT(elapsedTime, Constants::kKvstoreSyncInitialBackoff.count());
+  // discount 2ms. We have seen some-times elapsed time is
+  //     3999 ms instead of 4000 ms of Initial Backoff
+  EXPECT_GT(elapsedTime, (Constants::kKvstoreSyncInitialBackoff.count() - 2));
   EXPECT_LT(elapsedTime, Constants::kKvstoreSyncMaxBackoff.count());
 
   cmpPeers = storeA->getPeers(kTestingAreaName);