From 79db9acb359035ea18c677185269450b4fbe4b9e Mon Sep 17 00:00:00 2001 From: Tor Brede Vekterli Date: Fri, 5 May 2023 12:32:02 +0000 Subject: Address PR review comments --- storage/src/tests/distributor/check_condition_test.cpp | 13 +++++++++++++ storage/src/tests/distributor/putoperationtest.cpp | 2 ++ .../distributor/operations/external/putoperation.cpp | 4 ++-- 3 files changed, 17 insertions(+), 2 deletions(-) (limited to 'storage') diff --git a/storage/src/tests/distributor/check_condition_test.cpp b/storage/src/tests/distributor/check_condition_test.cpp index 7e42236f660..d4b7d7019d7 100644 --- a/storage/src/tests/distributor/check_condition_test.cpp +++ b/storage/src/tests/distributor/check_condition_test.cpp @@ -137,6 +137,8 @@ TEST_F(CheckConditionTest, starting_sends_condition_probe_gets) { auto cond = create_check_condition(); ASSERT_TRUE(cond); EXPECT_FALSE(cond->maybe_outcome()); + // Nothing should be sent prior to start_and_send() + ASSERT_EQ("", _sender.getCommands(true)); // We don't test too much of the Get functionality, as that's already covered by GetOperation tests. // But we test the main binding glue between the two components. cond->start_and_send(_sender); @@ -160,6 +162,17 @@ TEST_F(CheckConditionTest, condition_matching_completes_check_with_match_outcome }); } +TEST_F(CheckConditionTest, newest_document_version_is_authoritative_for_condition_match) { + test_cond_with_2_gets_sent([&](auto& cond) { + cond.handle_reply(_sender, make_matched_reply(0, 1001)); + cond.handle_reply(_sender, make_mismatched_reply(1, 1000)); + }, [&](auto& outcome) { + EXPECT_TRUE(outcome.matched_condition()); + EXPECT_FALSE(outcome.not_found()); + EXPECT_FALSE(outcome.failed()); + }); +} + TEST_F(CheckConditionTest, condition_mismatching_completes_check_with_mismatch_outcome) { test_cond_with_2_gets_sent([&](auto& cond) { cond.handle_reply(_sender, make_matched_reply(0, 1000)); diff --git a/storage/src/tests/distributor/putoperationtest.cpp b/storage/src/tests/distributor/putoperationtest.cpp index a69cf8ec54c..c60c5932709 100644 --- a/storage/src/tests/distributor/putoperationtest.cpp +++ b/storage/src/tests/distributor/putoperationtest.cpp @@ -703,6 +703,8 @@ TEST_F(PutOperationTest, matching_condition_probe_sends_unconditional_puts_to_al auto put_n0 = sent_put_command(3); EXPECT_FALSE(put_n0->hasTestAndSetCondition()); + EXPECT_EQ("", _sender.getReplies(true)); // No reply yet; content node Puts must be replied to first. + // Ensure replies are no longer routed to condition checker ASSERT_TRUE(_sender.replies().empty()); sendReply(2); // put to node 1 diff --git a/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp b/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp index 86b92336fb8..2e2eacd88c1 100644 --- a/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp +++ b/storage/src/vespa/storage/distributor/operations/external/putoperation.cpp @@ -159,9 +159,9 @@ void PutOperation::start_conditional_put(DistributorStripeMessageSender& sender) if (!_check_condition) { start_direct_put_dispatch(sender); } else { - // Inconsistent or non-existent replicas; might need write repair + // Inconsistent replicas; need write repair _check_condition->start_and_send(sender); - const auto& outcome = _check_condition->maybe_outcome(); // Might be done immediately ("no replicas"-case) + const auto& outcome = _check_condition->maybe_outcome(); // Might be done immediately if (outcome) { on_completed_check_condition(*outcome, sender); } -- cgit v1.2.3