You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2018/06/25 19:03:43 UTC
[kudu-CR] [rebalance-test] fix running the test on Ubuntu 18.04
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10812
Change subject: [rebalance-test] fix running the test on Ubuntu 18.04
......................................................................
[rebalance-test] fix running the test on Ubuntu 18.04
Updated custom equality operators for TableBalanceInfo and
ClusterBalanceInfo. With this update, the operator compares just the
contents of corresponding sub-fields of the ServersByCountMap type
(which is map<int32_t, string>), disregarding the order of elements for
the same key. That's the exact behavior desired: the ordering of
elements in the ServersByCountMap is not important in all scenarios
of the rebalance-test.
The motivation for this change is the fact that the test fails on
Ubuntu 18.04 without this patch. Implicitly, the reference results
relied on the order of elements in hash dictionary container used in
the implementation of the Rebalancer::KsckResultsToClusterBalanceInfo()
method. It seems the implementation of std::unordered_map in libstdc++
shipped with Ubuntu 18.04 has changed compared with older Linux distro
versions.
Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
---
M src/kudu/tools/rebalance-test.cc
1 file changed, 64 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10812/1
--
To view, visit http://gerrit.cloudera.org:8080/10812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
Gerrit-Change-Number: 10812
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [rebalance-test] fix running the test on Ubuntu 18.04
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10812 )
Change subject: [rebalance-test] fix running the test on Ubuntu 18.04
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10812/1/src/kudu/tools/rebalance-test.cc
File src/kudu/tools/rebalance-test.cc:
http://gerrit.cloudera.org:8080/#/c/10812/1/src/kudu/tools/rebalance-test.cc@22
PS1, Line 22: #include <iterator>
> warning: #includes are not sorted properly [llvm-include-order]
Done
http://gerrit.cloudera.org:8080/#/c/10812/1/src/kudu/tools/rebalance-test.cc@40
PS1, Line 40: using std::numeric_limits;
> warning: using decl 'numeric_limits' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/10812/1/src/kudu/tools/rebalance-test.cc@42
PS1, Line 42: using std::set;
> warning: using decl 'set' is unused [misc-unused-using-decls]
Done
--
To view, visit http://gerrit.cloudera.org:8080/10812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
Gerrit-Change-Number: 10812
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 20:11:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rebalance-test] fix running the test on Ubuntu 18.04
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10812
to look at the new patch set (#2).
Change subject: [rebalance-test] fix running the test on Ubuntu 18.04
......................................................................
[rebalance-test] fix running the test on Ubuntu 18.04
Updated custom equality operators for TableBalanceInfo and
ClusterBalanceInfo. With this update, the operator compares just the
contents of corresponding sub-fields of the ServersByCountMap type
(which is map<int32_t, string>), disregarding the order of elements for
the same key. That's the exact behavior desired: the ordering of
elements in the ServersByCountMap is not important in all scenarios
of the rebalance-test.
The motivation for this change is the fact that the test fails on
Ubuntu 18.04 without this patch. Implicitly, the reference results
relied on the order of elements in hash dictionary container used in
the implementation of the Rebalancer::KsckResultsToClusterBalanceInfo()
method. It seems the implementation of std::unordered_map in libstdc++
shipped with Ubuntu 18.04 has changed compared with older Linux distro
versions.
Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
---
M src/kudu/tools/rebalance-test.cc
1 file changed, 64 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10812/2
--
To view, visit http://gerrit.cloudera.org:8080/10812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
Gerrit-Change-Number: 10812
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [rebalance-test] fix running the test on Ubuntu 18.04
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10812 )
Change subject: [rebalance-test] fix running the test on Ubuntu 18.04
......................................................................
[rebalance-test] fix running the test on Ubuntu 18.04
Updated custom equality operators for TableBalanceInfo and
ClusterBalanceInfo. With this update, the operator compares just the
contents of corresponding sub-fields of the ServersByCountMap type
(which is map<int32_t, string>), disregarding the order of elements for
the same key. That's the exact behavior desired: the ordering of
elements in the ServersByCountMap is not important in all scenarios
of the rebalance-test.
The motivation for this change is the fact that the test fails on
Ubuntu 18.04 without this patch. Implicitly, the reference results
relied on the order of elements in hash dictionary container used in
the implementation of the Rebalancer::KsckResultsToClusterBalanceInfo()
method. It seems the implementation of std::unordered_map in libstdc++
shipped with Ubuntu 18.04 has changed compared with older Linux distro
versions.
Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
Reviewed-on: http://gerrit.cloudera.org:8080/10812
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/tools/rebalance-test.cc
1 file changed, 64 insertions(+), 7 deletions(-)
Approvals:
Alexey Serbin: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
Gerrit-Change-Number: 10812
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [rebalance-test] fix running the test on Ubuntu 18.04
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10812 )
Change subject: [rebalance-test] fix running the test on Ubuntu 18.04
......................................................................
Patch Set 3: Verified+1
Unrelated flakes.
--
To view, visit http://gerrit.cloudera.org:8080/10812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
Gerrit-Change-Number: 10812
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 25 Jun 2018 20:54:59 +0000
Gerrit-HasComments: No
[kudu-CR] [rebalance-test] fix running the test on Ubuntu 18.04
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10812 )
Change subject: [rebalance-test] fix running the test on Ubuntu 18.04
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
The test now passes on Ubuntu 18.04. Thanks!
http://gerrit.cloudera.org:8080/#/c/10812/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10812/3//COMMIT_MSG@17
PS3, Line 17: The motivation for this change is the fact that the test fails on
: Ubuntu 18.04 without this patch. Implicitly, the reference results
: relied on the order of elements in hash dictionary container used in
: the implementation of the Rebalancer::KsckResultsToClusterBalanceInfo()
: method. It seems the implementation of std::unordered_map in libstdc++
: shipped with Ubuntu 18.04 has changed compared with older Linux distro
: versions.
FWIW, I changed all of the unordered_maps and unordered_sets in the tools/ directory to regular maps and sets, and the test still failed.
If we're iterating on hashed containers, perhaps we should be using unhashed containers instead? Or do we need the O(1) lookups provided by the hashed containers?
--
To view, visit http://gerrit.cloudera.org:8080/10812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
Gerrit-Change-Number: 10812
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 16:55:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] [rebalance-test] fix running the test on Ubuntu 18.04
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10812
to look at the new patch set (#3).
Change subject: [rebalance-test] fix running the test on Ubuntu 18.04
......................................................................
[rebalance-test] fix running the test on Ubuntu 18.04
Updated custom equality operators for TableBalanceInfo and
ClusterBalanceInfo. With this update, the operator compares just the
contents of corresponding sub-fields of the ServersByCountMap type
(which is map<int32_t, string>), disregarding the order of elements for
the same key. That's the exact behavior desired: the ordering of
elements in the ServersByCountMap is not important in all scenarios
of the rebalance-test.
The motivation for this change is the fact that the test fails on
Ubuntu 18.04 without this patch. Implicitly, the reference results
relied on the order of elements in hash dictionary container used in
the implementation of the Rebalancer::KsckResultsToClusterBalanceInfo()
method. It seems the implementation of std::unordered_map in libstdc++
shipped with Ubuntu 18.04 has changed compared with older Linux distro
versions.
Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
---
M src/kudu/tools/rebalance-test.cc
1 file changed, 64 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/10812/3
--
To view, visit http://gerrit.cloudera.org:8080/10812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
Gerrit-Change-Number: 10812
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [rebalance-test] fix running the test on Ubuntu 18.04
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10812 )
Change subject: [rebalance-test] fix running the test on Ubuntu 18.04
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/10812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
Gerrit-Change-Number: 10812
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [rebalance-test] fix running the test on Ubuntu 18.04
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10812 )
Change subject: [rebalance-test] fix running the test on Ubuntu 18.04
......................................................................
Patch Set 3:
(1 comment)
Thank you for the review.
http://gerrit.cloudera.org:8080/#/c/10812/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10812/3//COMMIT_MSG@17
PS3, Line 17: The motivation for this change is the fact that the test fails on
: Ubuntu 18.04 without this patch. Implicitly, the reference results
: relied on the order of elements in hash dictionary container used in
: the implementation of the Rebalancer::KsckResultsToClusterBalanceInfo()
: method. It seems the implementation of std::unordered_map in libstdc++
: shipped with Ubuntu 18.04 has changed compared with older Linux distro
: versions.
> FWIW, I changed all of the unordered_maps and unordered_sets in the tools/
Yep, I also did that change in the tools/ directory. In addition to that change it's necessary to change the reference results in the rebalance-test.cc so the strings for every integer key would be ordered appropriately. I didn't change the ordering of corresponding keys in the reference results because a) with this change the custom comparison operator 'normalizes' the input anyway b) the essence of this patch would not less obvious.
I think it's better to use hashed dictionaries vs tree dictionaries because of faster lookup speed: O(1) instead of O(log(n)) since number of tablets might be high enough. I didn't evaluate how much gain it gives us in real world scenarios, but I suspect there is some. As for the iterating over those maps, the order of elements there is not crucial except for the reference results comparison. We talked about that with Will and decided to keep hashed maps. If we find the regular tree maps are better, we can easily switch at any point.
--
To view, visit http://gerrit.cloudera.org:8080/10812
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie4dc5d22bab86f6ab61f18f05e45a063bbf38eba
Gerrit-Change-Number: 10812
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 27 Jun 2018 17:16:28 +0000
Gerrit-HasComments: Yes