Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20781 )
Change subject: KUDU-3532: Fix range aware replica placement bug ...................................................................... Patch Set 1: (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 with two sets of items: one to select from and another is to avoid selecting from, where the current code, prior to this patch, had an improper assumption of the relations between the two sets, no? Let's put it this way: if all those container sizes were signed integers, and no conversion was involved, wouldn't the code produce wrong results in that case then? 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 last? It's great to have higher-level scenarios based on PlacementPolicy::SelectReplica(), but wouldn't we be more sure in the behavior of PlacementPolicy::SelectReplica() if we had a unit test checking against basic invariants? 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 'excluded' might be quite an arbitrary set, having no relation to 'ts_descs'? 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 it be better to refactor ReservoirSample() to make it work properly with unsigned types? BTW, current ReservoirSample's implementation is using Random::Uniform() call that takes an unsigned integer as input and returns an unsigned integer as well. -- 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: 1 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 13 Dec 2023 01:54:42 +0000 Gerrit-HasComments: Yes
