Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20781 )
Change subject: KUDU-3532: Fix range aware replica placement bug ...................................................................... Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/20781/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20781/1//COMMIT_MSG@9 PS1, Line 9: An implicit conversion from unsigned long to int caused > IIUC, the actual root case was a mistake in the logic of the code working w Yep that's accurate. I updated the commit message of the downstream commits to reflect that, this was already merged before I saw this feedback. I'll update the upstream and downstream jiras to properly reflect the issue. http://gerrit.cloudera.org:8080/#/c/20781/1/src/kudu/master/placement_policy-test.cc File src/kudu/master/placement_policy-test.cc: PS1: > Maybe, it's time to add a unit test for PlacementPolicy::SelectReplica() at Done http://gerrit.cloudera.org:8080/#/c/20781/1/src/kudu/master/placement_policy.cc File src/kudu/master/placement_policy.cc: http://gerrit.cloudera.org:8080/#/c/20781/1/src/kudu/master/placement_policy.cc@376 PS1, Line 376: shared_ptr<TSDescriptor> PlacementPolicy::SelectReplica( > Does it make sense to modify the comment above to explicitly state that 'ex Done http://gerrit.cloudera.org:8080/#/c/20781/1/src/kudu/master/placement_policy.cc@384 PS1, Line 384: const int ts_size = static_cast<int>(ts_descs.size()); : CHECK_GE(ts_size, 0); > It's highly unlikely to have a cluster with more than 2^31 nodes, but could Done -- To view, visit http://gerrit.cloudera.org:8080/20781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5d696d58965590a9f91f8b1b59f23225bbad8ee Gerrit-Change-Number: 20781 Gerrit-PatchSet: 2 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 21 Dec 2023 06:32:09 +0000 Gerrit-HasComments: Yes
