diff --git a/CHANGELOG b/CHANGELOG index 40df2e6e35..59b3a2c737 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,9 @@ v3.5.1 (XXXX-XX-XX) ------------------- +* Fixed internal issue #4378: fix bug in MoveShard::abort which causes a + duplicate entry in the follower list. + * Fixed cut'n'pasting code from the documentation into arangosh. * Fixed issue #9652: fix ArangoSearch wrong name collision and raising diff --git a/arangod/Agency/MoveShard.cpp b/arangod/Agency/MoveShard.cpp index b1b66702c6..2ab5009213 100644 --- a/arangod/Agency/MoveShard.cpp +++ b/arangod/Agency/MoveShard.cpp @@ -840,20 +840,22 @@ arangodb::Result MoveShard::abort(std::string const& reason) { if (_isLeader) { // All changes to Plan for all shards: doForAllShards(_snapshot, _database, shardsLikeMe, - [this, &trx](Slice plan, Slice current, std::string& planPath, std::string& curPath) { - // Restore leader to be _from: - trx.add(VPackValue(planPath)); - { - VPackArrayBuilder guard(&trx); - trx.add(VPackValue(_from)); - VPackArrayIterator iter(plan); - ++iter; // skip the first - while (iter.valid()) { - trx.add(iter.value()); - ++iter; - } - } - }); + [this, &trx](Slice plan, Slice current, std::string& planPath, std::string& curPath) { + // Restore leader to be _from: + trx.add(VPackValue(planPath)); + { + VPackArrayBuilder guard(&trx); + trx.add(VPackValue(_from)); + for (auto const& srv : VPackArrayIterator(plan)) { + // from could be in plan as or <_from>. Exclude to server always. + if (srv.isEqualString(_from) || srv.isEqualString("_" + _from) || srv.isEqualString(_to)) { + continue ; + } + trx.add(srv); + } + trx.add(VPackValue(_to)); + } + }); } else { // All changes to Plan for all shards: doForAllShards(_snapshot, _database, shardsLikeMe, @@ -863,7 +865,7 @@ arangodb::Result MoveShard::abort(std::string const& reason) { { VPackArrayBuilder guard(&trx); for (auto const& srv : VPackArrayIterator(plan)) { - if (srv.copyString() != _to) { + if (false == srv.isEqualString(_to)) { trx.add(srv); } } diff --git a/tests/Agency/MoveShardTest.cpp b/tests/Agency/MoveShardTest.cpp index d9604c25ef..578dfa1099 100644 --- a/tests/Agency/MoveShardTest.cpp +++ b/tests/Agency/MoveShardTest.cpp @@ -2057,6 +2057,85 @@ TEST_F(MoveShardTest, aborting_the_job_while_a_leader_transition_is_in_progress_ Verify(Method(mockAgent, write)); } +TEST_F(MoveShardTest, aborting_the_job_while_the_new_leader_is_already_in_place_should_not_break_plan) { + std::function(VPackSlice const&, std::string const&)> createTestStructure = [&](VPackSlice const& s, std::string const& path) { + std::unique_ptr builder; + builder.reset(new VPackBuilder()); + if (s.isObject()) { + builder->add(VPackValue(VPackValueType::Object)); + for (auto const& it: VPackObjectIterator(s)) { + auto childBuilder = createTestStructure(it.value, path + "/" + it.key.copyString()); + if (childBuilder) { + builder->add(it.key.copyString(), childBuilder->slice()); + } + } + + if (path == "/arango/Target/Pending") { + VPackBuilder pendingJob; + { + VPackObjectBuilder b(&pendingJob); + auto plainJob = createJob(COLLECTION, SHARD_LEADER, FREE_SERVER); + for (auto const& it: VPackObjectIterator(plainJob.slice())) { + pendingJob.add(it.key.copyString(), it.value); + } + pendingJob.add("timeCreated", VPackValue(timepointToString(std::chrono::system_clock::now()))); + } + builder->add(jobId, pendingJob.slice()); + } else if (path == "/arango/Supervision/DBServers") { + builder->add(FREE_SERVER, VPackValue("1")); + } else if (path == "/arango/Supervision/Shards") { + builder->add(SHARD, VPackValue("1")); + } + builder->close(); + } else { + if (path == "/arango/Current/Collections/" + DATABASE + "/" + COLLECTION + "/" + SHARD + "/servers") { + builder->add(VPackValue(VPackValueType::Array)); + builder->add(VPackValue("_" + SHARD_LEADER)); + builder->add(VPackValue(SHARD_FOLLOWER1)); + builder->close(); + } else if (path == "/arango/Plan/Collections/" + DATABASE + "/" + COLLECTION + "/shards/" + SHARD) { + builder->add(VPackValue(VPackValueType::Array)); + builder->add(VPackValue(FREE_SERVER)); + builder->add(VPackValue(SHARD_LEADER)); + builder->add(VPackValue(SHARD_FOLLOWER1)); + builder->close(); + } else { + builder->add(s); + } + } + return builder; + }; + + Mock mockAgent; + When(Method(mockAgent, waitFor)).AlwaysReturn(); + When(Method(mockAgent, write)).Do([&](query_t const& q, consensus::AgentInterface::WriteMode w) -> write_ret_t { + + auto writes = q->slice()[0][0]; + EXPECT_TRUE(writes.get("/arango/Target/Pending/1").get("op").copyString() == "delete"); + EXPECT_TRUE(q->slice()[0].length() == 2); // Precondition: to Server not leader yet + EXPECT_TRUE(writes.get("/arango/Supervision/DBServers/" + FREE_SERVER).get("op").copyString() == "delete"); + EXPECT_TRUE(writes.get("/arango/Supervision/Shards/" + SHARD).get("op").copyString() == "delete"); + EXPECT_TRUE(std::string(writes.get("/arango/Plan/Collections/" + DATABASE + "/" + COLLECTION + "/shards/" + SHARD).typeName()) == "array"); + // well apparently this job is not responsible to cleanup its mess + EXPECT_TRUE(writes.get("/arango/Plan/Collections/" + DATABASE + "/" + COLLECTION + "/shards/" + SHARD).length() >= 3); + EXPECT_TRUE(writes.get("/arango/Plan/Collections/" + DATABASE + "/" + COLLECTION + "/shards/" + SHARD)[0].copyString() == SHARD_LEADER); + EXPECT_TRUE(writes.get("/arango/Plan/Collections/" + DATABASE + "/" + COLLECTION + "/shards/" + SHARD)[1].copyString() == SHARD_FOLLOWER1); + EXPECT_TRUE(writes.get("/arango/Plan/Collections/" + DATABASE + "/" + COLLECTION + "/shards/" + SHARD)[2].copyString() == FREE_SERVER); + EXPECT_TRUE(std::string(writes.get("/arango/Target/Failed/1").typeName()) == "object"); + + return fakeWriteResult; + }); + AgentInterface& agent = mockAgent.get(); + + auto builder = createTestStructure(baseStructure.toBuilder().slice(), ""); + EXPECT_TRUE(builder); + Node agency = createAgencyFromBuilder(*builder); + + auto moveShard = MoveShard(agency, &agent, PENDING, jobId); + moveShard.abort("test abort"); + Verify(Method(mockAgent,write)); +} + TEST_F(MoveShardTest, if_we_are_ready_to_resign_the_old_server_then_finally_move_to_the_new_leader) { std::function(VPackSlice const&, std::string const&)> createTestStructure = [&](VPackSlice const& s, std::string const& path) {