diff options
author | Tor Brede Vekterli <vekterli@yahooinc.com> | 2021-12-13 14:42:36 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2021-12-13 14:42:36 +0100 |
commit | e4033438da88cc24ceefd440d51ba18909682736 (patch) | |
tree | 90cb1571bebda2b234cd8f3bf23bf5ead1f6415e | |
parent | 82397b67bcbe865283cece305037577294a86fa2 (diff) | |
parent | 7bb18747e7467ace9f713d97b04534c287f4033f (diff) |
Merge pull request #20491 from vespa-engine/revert-20382-vekterli/treat-empty-replica-subset-as-inconsistent-for-get-operations
Revert "Treat empty replica subset as inconsistent for GetOperation [run-systemtest]"
3 files changed, 0 insertions, 26 deletions
diff --git a/storage/src/tests/distributor/getoperationtest.cpp b/storage/src/tests/distributor/getoperationtest.cpp index 9fecb005659..dfe4f09de3f 100644 --- a/storage/src/tests/distributor/getoperationtest.cpp +++ b/storage/src/tests/distributor/getoperationtest.cpp @@ -267,26 +267,6 @@ TEST_F(GetOperationTest, send_to_all_invalid_nodes_when_inconsistent) { EXPECT_EQ("newauthor", getLastReplyAuthor()); } -// GetOperation document-level consistency checks are used by the multi-phase update -// logic to see if we can fall back to a fast path even though not all replicas are in sync. -// Empty replicas are not considered part of the send-set, so only looking at replies from -// replicas _sent_ to will not detect this case. -// If we haphazardly treat an empty replicas as implicitly being in sync we risk triggering -// undetectable inconsistencies at the document level. This can happen if we send create-if-missing -// updates to an empty replica as well as a non-empty replica, and the document exists in the -// latter replica. The document would then be implicitly created on the empty replica with the -// same timestamp as that of the non-empty one, even though their contents would almost -// certainly differ. -TEST_F(GetOperationTest, get_not_sent_to_empty_replicas_but_bucket_tagged_as_inconsistent) { - setClusterState("distributor:1 storage:4"); - addNodesToBucketDB(bucketId, "2=0/0/0,3=1/2/3"); - sendGet(); - ASSERT_EQ("Get => 3", _sender.getCommands(true)); - ASSERT_NO_FATAL_FAILURE(sendReply(0, api::ReturnCode::OK, "newauthor", 2)); - EXPECT_FALSE(op->any_replicas_failed()); - EXPECT_FALSE(last_reply_had_consistent_replicas()); -} - TEST_F(GetOperationTest, inconsistent_split) { setClusterState("distributor:1 storage:4"); diff --git a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp index 868de8d0ae2..06872cadde6 100644 --- a/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/getoperation.cpp @@ -267,11 +267,6 @@ GetOperation::assignTargetNodeGroups(const BucketDatabase::ReadGuard& read_guard _responses[GroupId(e.getBucketId(), copy.getChecksum(), copy.getNode())].emplace_back(copy); } else if (!copy.empty()) { _responses[GroupId(e.getBucketId(), copy.getChecksum(), -1)].emplace_back(copy); - } else { // empty replica - // We must treat a bucket with empty replicas as inherently inconsistent. - // See GetOperationTest::get_not_sent_to_empty_replicas_but_bucket_tagged_as_inconsistent for - // rationale as to why this is the case. - _has_replica_inconsistency = true; } } } diff --git a/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp index 5233e5678fa..a16eef0ab6f 100644 --- a/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/updateoperation.cpp @@ -90,7 +90,6 @@ UpdateOperation::onStart(DistributorStripeMessageSender& sender) // An UpdateOperation should only be started iff all replicas are consistent // with each other, so sampling a single replica should be equal to sampling them all. - // FIXME this no longer holds when replicas are consistent at the _document_ level but not at the _bucket_ level. assert(_entries[0].getBucketInfo().getNodeCount() > 0); // Empty buckets are not allowed _infoAtSendTime = _entries[0].getBucketInfo().getNodeRef(0).getBucketInfo(); |