From commits-return-6722-archive-asf-public=cust-asf.ponee.io@kudu.apache.org Tue Nov 6 23:38:31 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 8EEB018067E for ; Tue, 6 Nov 2018 23:38:30 +0100 (CET) Received: (qmail 1971 invoked by uid 500); 6 Nov 2018 22:38:29 -0000 Mailing-List: contact commits-help@kudu.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@kudu.apache.org Delivered-To: mailing list commits@kudu.apache.org Received: (qmail 1958 invoked by uid 99); 6 Nov 2018 22:38:29 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 06 Nov 2018 22:38:29 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 22644E1200; Tue, 6 Nov 2018 22:38:29 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: alexey@apache.org To: commits@kudu.apache.org Date: Tue, 06 Nov 2018 22:38:31 -0000 Message-Id: <1105021b73374a21a0f53c3821483dd9@git.apache.org> In-Reply-To: <8e230757f23644cebb9d6454ee1bf7a3@git.apache.org> References: <8e230757f23644cebb9d6454ee1bf7a3@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: [3/3] kudu git commit: [tools] reference comparison mode for rebalancing algo tests [tools] reference comparison mode for rebalancing algo tests Introduced comparison mode for rebalancing algorithms' tests. For current rebalancing algorithms, it's natural to re-order contiguous moves of the same weight. That's because of: * Iterating over elements of a hash container keyed by the weight of a move. * Randomly choosing among multiple options of the same weight. This patch adds MovesOrderingComparison::IGNORE option into one test configuration of the RebalanceAlgoUnitTest.LocationBalancingSimpleST scenario. That fixes the breakage of the test on Ubuntu 18. Change-Id: I8363f013b5bf8caa3e3b967c64eccca95c763a91 Reviewed-on: http://gerrit.cloudera.org:8080/11870 Tested-by: Kudu Jenkins Reviewed-by: Will Berkeley Project: http://git-wip-us.apache.org/repos/asf/kudu/repo Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/6ce61d6e Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/6ce61d6e Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/6ce61d6e Branch: refs/heads/master Commit: 6ce61d6e67f3cb54ed034fda9c5dd443970ea454 Parents: 8e9345a Author: Alexey Serbin Authored: Fri Nov 2 12:06:29 2018 -0700 Committer: Alexey Serbin Committed: Tue Nov 6 22:28:12 2018 +0000 ---------------------------------------------------------------------- src/kudu/tools/rebalance_algo-test.cc | 63 ++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 4 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kudu/blob/6ce61d6e/src/kudu/tools/rebalance_algo-test.cc ---------------------------------------------------------------------- diff --git a/src/kudu/tools/rebalance_algo-test.cc b/src/kudu/tools/rebalance_algo-test.cc index 3271339..6a5e5a8 100644 --- a/src/kudu/tools/rebalance_algo-test.cc +++ b/src/kudu/tools/rebalance_algo-test.cc @@ -17,6 +17,7 @@ #include "kudu/tools/rebalance_algo.h" +#include #include #include #include @@ -67,6 +68,7 @@ using std::endl; using std::ostream; using std::ostringstream; using std::set; +using std::sort; using std::string; using std::unordered_map; using std::vector; @@ -84,6 +86,23 @@ struct TablePerServerReplicas { const vector num_replicas_by_server; }; +// Whether the order of the moves in the reference results should be verified +// against the actual moves. +enum class MovesOrderingComparison { + IGNORE, + VERIFY, +}; +struct ReferenceComparisonOptions { + // Constructor to initialize the options by default. + // NOLINTNEXTLINE(google-explicit-constructor) + ReferenceComparisonOptions(MovesOrderingComparison moves_ordering = + MovesOrderingComparison::VERIFY) + : moves_ordering(moves_ordering) { + } + + const MovesOrderingComparison moves_ordering; +}; + // Structure to describe rebalancing-related state of the cluster expressively // enough for the tests. struct TestClusterConfig { @@ -104,6 +123,9 @@ struct TestClusterConfig { // The expected replica movements: the reference output of the algorithm // to compare with. const vector expected_moves; + + // Options controlling how the reference and the actual results are compared. + const ReferenceComparisonOptions ref_comparison_options; }; bool operator==(const TableReplicaMove& lhs, const TableReplicaMove& rhs) { @@ -201,7 +223,40 @@ void VerifyLocationRebalancingMoves(const TestClusterConfig& cfg) { LocationBalancingAlgo algo; ASSERT_OK(algo.GetNextMoves(ci, 0, &moves)); } - EXPECT_EQ(cfg.expected_moves, moves); + switch (cfg.ref_comparison_options.moves_ordering) { + case MovesOrderingComparison::IGNORE: + { + // The case when the order of moves is not important. For the + // rebalancing algorithms, it's natural to re-order contiguous moves + // of the same weight. This is because of: + // a) randomly choosing among multiple options of the same weight + // b) iterating over elements of a hash container keyed by the weight + // of a move. + // Here it's necessary to normalize both the reference and the actual + // results before performing element-to-element comparison. + vector ref_moves(cfg.expected_moves); + constexpr auto kMovesComparator = [](const TableReplicaMove& lhs, + const TableReplicaMove& rhs) { + if (lhs.table_id != rhs.table_id) { + return lhs.table_id < rhs.table_id; + } + if (lhs.from != rhs.from) { + return lhs.from < rhs.from; + } + return lhs.to < rhs.to; + }; + sort(ref_moves.begin(), ref_moves.end(), kMovesComparator); + sort(moves.begin(), moves.end(), kMovesComparator); + EXPECT_EQ(ref_moves, moves); + } + break; + case MovesOrderingComparison::VERIFY: + EXPECT_EQ(cfg.expected_moves, moves); + break; + default: + FAIL() << "unexpected reference comparison style"; + break; + } } // Is 'cbi' balanced according to the two-dimensional greedy algorithm? @@ -984,13 +1039,13 @@ TEST(RebalanceAlgoUnitTest, LocationBalancingSimpleST) { }, { "0", "1", "2", }, { { "A", { 6, 0, 0, } }, }, - // TODO(aserbin): what about ordering? { - { "A", "0", "2" }, { "A", "0", "1" }, + { "A", "0", "2" }, { "A", "0", "1" }, { "A", "0", "2" }, - } + }, + { MovesOrderingComparison::IGNORE } }, { {