MB-47387: Make ep_warmup_thread indicate all threads finished

Have ep_warmup_thread indicate that all warmup threads have actually
finished (which in turn means that DcpConsumers can be created).
ns_server can then create a DcpConsumer and expect that it should not
return temporary_failure.

Change-Id: Icd6c587001eab2d7821e09673a5652675f9817d0
Reviewed-on: http://review.couchbase.org/c/kv_engine/+/163886
Tested-by: Build Bot <[email protected]>
Reviewed-by: James H <[email protected]>
Reviewed-by: Dave Rigby <[email protected]>

Author: BenHuddleston

engines/ep/src/warmup.cc

@@ -1902,7 +1902,7 @@ void Warmup::addStats(const AddStatFn& add_stat, const CookieIface* c) const {
     addStat(nullptr, "enabled", add_stat, c);
     const char* stateName = state.toString();
     addStat("state", stateName, add_stat, c);
-    if (finishedLoading.load()) {
+    if (isComplete()) {
         addStat("thread", "complete", add_stat, c);
     } else {
         addStat("thread", "running", add_stat, c);

engines/ep/src/warmup.h

@@ -214,8 +214,11 @@ class WarmupState {
  *
  * Note that once warmup is complete (Warmup::isFinishedLoading() returns true)
  * the warmup will conclude the phase and short-cut to Done. When
- * Warmup::isFinishedLoading() returns true: 1) all CRUD operations are fully
- * processed. 2) DCP consumers can be created.
+ * Warmup::isFinishedLoading() returns true all CRUD operations are fully
+ * processed. When Warmup::isComplete() returns true DCP consumers can be
+ * created. isComplete() will return true later than isFinishedLoading() as we
+ * need to marshall all of the warmup threads before allowing DCP Consumers to
+ * prevent race conditions with rollback.
  */
 class Warmup {
 public:

engines/ep/tests/module_tests/evp_store_warmup_test.cc

@@ -2811,6 +2811,25 @@ TEST_F(WarmupTest, ConsumerDuringWarmup) {
     runNextTask(readerQueue);
     EXPECT_EQ(warmupPtr->getWarmupState(), WarmupState::State::LoadingData);
 
+    // ns_server gets stats to determine when to set up DcpConsumers. The stat
+    // ep_warmup_thread encapsulates the state of warmup threads and returns a
+    // value of either "running" or "complete". Check here that it is "running"
+    // as we will temp_fail a DcpConsumer creation.
+    {
+        bool threadStatAdded;
+        auto dummyAddStats = [&threadStatAdded](std::string_view key,
+                                                std::string_view value,
+                                                const void*) {
+            if (key == "ep_warmup_thread") {
+                EXPECT_EQ("running", value);
+                threadStatAdded = true;
+            }
+        };
+        EXPECT_EQ(cb::engine_errc::success,
+                  engine->get_stats(*cookie, "warmup", {}, dummyAddStats));
+        EXPECT_TRUE(threadStatAdded);
+    }
+
     // Still fails, not finished warmup
     EXPECT_EQ(cb::engine_errc::temporary_failure,
               engine->dcpOpen(cookie,
@@ -2828,6 +2847,23 @@ TEST_F(WarmupTest, ConsumerDuringWarmup) {
     // finish warmup
     runNextTask(readerQueue);
 
+    // Now that all warmup threads have complete, ep_warmup_thread should return
+    // "complete" indicating to ns_server that they can now create a DcpConsumer
+    {
+        bool threadStatAdded;
+        auto dummyAddStats = [&threadStatAdded](std::string_view key,
+                                                std::string_view value,
+                                                const void*) {
+            if (key == "ep_warmup_thread") {
+                EXPECT_EQ("complete", value);
+                threadStatAdded = true;
+            }
+        };
+        EXPECT_EQ(cb::engine_errc::success,
+                  engine->get_stats(*cookie, "warmup", {}, dummyAddStats));
+        EXPECT_TRUE(threadStatAdded);
+    }
+
     // Opening the connection should now work
     EXPECT_EQ(cb::engine_errc::success,
               engine->dcpOpen(cookie,