Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/14533 )
Change subject: KUDU-2962 Fix kudu::itest::FindTabletFollowers() test utility function ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc File src/kudu/integration-tests/cluster_itest_util.cc: http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@a649 PS6, Line 649: : : : : : : : : > Could we have done something kind of simple like have FindTabletLeader() re Ack http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@648 PS6, Line 648: // Fills the out parameter "followers" with the tablet servers hosting the "tablet_id". : // Non-empty "tablet_servers" is expected to include all the tablet servers and not subset : // of tablet servers that host the "tablet_id". > nit: could you add something to the effect of, "Returns an error if the tab Done http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@667 PS6, Line 667: _or_peers_f > nit: could you call this tserver_uuid instead? Tablet IDs are also UUIDs so Done http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@671 PS6, Line 671: return Status::TimedOut(Substitute("Unable to find followers for tablet $0 after $1.", : tablet_id, (now - start).ToString())); : } : const auto& tserver_uuid = entry.first; : c > nit: maybe move this up to the top before we do anything in this loop Done http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@692 PS6, Line 692: > nit: maybe "current_peer_uuids" or "peer_uuids"? We use UUIDs in a few plac Done http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@714 PS6, Line 714: > nit: how about calling this "leader_peer_uuids" or somesuch so it's clear t Calling it leader_peer_uuids would suggest they are peers as reported by the leader which is not the case here. Instead using peer_uuids along with curr_peer_uuids inside the loop with the existing comment L660 explaining the purpose. http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@736 PS6, Line 736: "Not all peers reported by tablet servers are part of the supplied tablet servers.")); : } : : for (const auto& tserver_uuid : peer_uuids) { : if (tserver_uuid != leader_uuid) { : followers->emplace_back(FindOrDie(tablet_servers, tserver_uuid)); : > nit: How about iterating through `tablet_peer_uuids` and using FindOrDie() Good point. Done -- To view, visit http://gerrit.cloudera.org:8080/14533 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I94146873b09cb95c54175d16305b713365cfeaa8 Gerrit-Change-Number: 14533 Gerrit-PatchSet: 7 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Oct 2019 17:36:10 +0000 Gerrit-HasComments: Yes
