MB-24066: Partial revert "MB-22178: Don't use opencheckpointid to determine if in...

Revert functional changes in
Change-Id: Ia977d6bf90e54fd1ceb8db4a9088b19d94d4bc8c,
which although addressed the rollback bug described in MB-22178, caused
the bug described in MB-24066.

The tests added in Ia977d6bf90e54fd1ceb8db4a9088b19d94d4bc8c remain but
the rollback test has been disabled as it attempts a streamRequest when
the vbucket has a opendcheckpointid of 0.

The test will be re-enabled and modified with a follow-up patch that
addresses the bug described in MB-22178.

Change-Id: Ifd11c77a10e4ebe571c50e5d518403b423c3dfa5
Reviewed-on: http://review.couchbase.org/77604
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
Tested-by: Build Bot <build@couchbase.com>

diff --git a/src/dcp/producer.cc b/src/dcp/producer.cc
index 5295986..2b3cf4f 100644 (file)
--- a/src/dcp/producer.cc
+++ b/src/dcp/producer.cc
@@ -199,7 +199,7 @@ ENGINE_ERROR_CODE DcpProducer::streamRequest(uint32_t flags,
         return ENGINE_NOT_MY_VBUCKET;
     }
 
-    if (vb->isBackfillPhase()) {
+    if (vb->checkpointManager.getOpenCheckpointId() == 0) {
         LOG(EXTENSION_LOG_WARNING, "%s (vb %d) Stream request failed because "
             "this vbucket is in backfill state", logHeader(), vbucket);
         return ENGINE_TMPFAIL;

diff --git a/src/dcp/stream.cc b/src/dcp/stream.cc
index 93e95b8..24b930b 100644 (file)
--- a/src/dcp/stream.cc
+++ b/src/dcp/stream.cc
@@ -1810,16 +1810,8 @@ void PassiveStream::processMarker(SnapshotMarker* marker) {
             vb->checkpointManager.setBackfillPhase(cur_snapshot_start.load(),
                                                    cur_snapshot_end.load());
         } else {
-            if (marker->getFlags() & MARKER_FLAG_CHK || vb->isBackfillPhase()) {
-                    // Instead of checking isBackfillPhase() an earlier patch
-                    // http://review.couchbase.org/#/c/39960/ changed the check
-                    // to (vb->checkpointManager.getOpenCheckpointId() == 0).
-                    // This change has been reverted in favor of using the
-                    // isBackfillPhase() check as it is believed to be safer and
-                    // makes the logic easier to understand.  However if an
-                    // issues such as that seen in MB-11786 occurs then it maybe
-                    // worth investigating the impact of changing the check to
-                    // use (vb->checkpointManager.getOpenCheckpointId() == 0)
+            if (marker->getFlags() & MARKER_FLAG_CHK ||
+                    vb->checkpointManager.getOpenCheckpointId() == 0) {
                 vb->checkpointManager.createSnapshot(cur_snapshot_start.load(),
                                                      cur_snapshot_end.load());
             } else {

diff --git a/src/ep.cc b/src/ep.cc
index 04a7f6f..dcd2c78 100644 (file)
--- a/src/ep.cc
+++ b/src/ep.cc
@@ -4003,7 +4003,6 @@ EventuallyPersistentStore::rollback(uint16_t vbid,
                 vb->checkpointManager.clear(vb, result.highSeqno);
                 vb->setPersistedSnapshot(result.snapStartSeqno, result.snapEndSeqno);
                 vb->incrRollbackItemCount(prevHighSeqno - result.highSeqno);
-                vb->setBackfillPhase(false);
                 return ENGINE_SUCCESS;
             }
         }

diff --git a/tests/module_tests/evp_store_rollback_test.cc b/tests/module_tests/evp_store_rollback_test.cc
index 58ca61d..b4d9ffc 100644 (file)
--- a/tests/module_tests/evp_store_rollback_test.cc
+++ b/tests/module_tests/evp_store_rollback_test.cc
@@ -283,7 +283,7 @@ TEST_P(RollbackTest, RollbackToMiddleOfAnUnPersistedSnapshot) {
  * vbucket was in backfilling state.  This caused the producer stream
  * request to be stuck waiting for backfilling to complete.
  */
-TEST_P(RollbackTest, MB21784) {
+TEST_P(RollbackTest, DISABLED_MB21784) {
     // Make the vbucket a replica
     store->setVBucketState(vbid, vbucket_state_replica, false);
     // Perform a rollback