Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11862 )
Change subject: [rebalancer] location-aware rebalancer (part 9/n) ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/placement_policy_util-test.cc File src/kudu/tools/placement_policy_util-test.cc: http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/placement_policy_util-test.cc@142 PS2, Line 142: default comparision : // operator There's a default comparison operator in C++? I didn't think there was. How it should it work? I don't think we should use it, even in a test. I think tuples have a comparison operator if their components do, though. http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/placement_policy_util-test.cc@143 PS2, Line 143: nonsence nonsense http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc File src/kudu/tools/rebalancer.cc: http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@129 PS2, Line 129: iformation information http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@137 PS2, Line 137: vilations violations http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@138 PS2, Line 138: RETURN_NOT_OK(PrintPolicyViolationInfo(raw_info, out)); It looks like the output could be kind of long. Can we move placement policy violations to the bottom, where they're most likely to be seen when this is run from the command line. Alternatively, you can leave them at the top, but print an extra message at the bottom if there are placement policy violations, one directing users to look up here for details. http://gerrit.cloudera.org:8080/#/c/11862/2/src/kudu/tools/rebalancer.cc@145 PS2, Line 145: sorated sorted -- To view, visit http://gerrit.cloudera.org:8080/11862 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8407e9f8cf6b41a6aeb075372d852125d9739e08 Gerrit-Change-Number: 11862 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 02 Nov 2018 18:41:59 +0000 Gerrit-HasComments: Yes
