Andrew Wong 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 6: (7 comments) Looks good, mostly nits from me. 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() return the leader's cstate, checked the leader's followers against 'tablet_servers', and then returned the followers? Probably not as robust as what you have here, but another option, I think. 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 tablet servers do not have consensus or cannot be reached". http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@667 PS6, Line 667: tablet_uuid nit: could you call this tserver_uuid instead? Tablet IDs are also UUIDs so tablet_uuid could easily be confused http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@671 PS6, Line 671: const auto now = MonoTime::Now(); : if (now > deadline) { : return Status::TimedOut(Substitute("Unable to find followers for tablet $0 after $1.", : tablet_id, (now - start).ToString())); : } nit: maybe move this up to the top before we do anything in this loop http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@692 PS6, Line 692: uuids nit: maybe "current_peer_uuids" or "peer_uuids"? We use UUIDs in a few places, so picking a more specific name can really help understandability. http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@714 PS6, Line 714: tablet_peer_uuids nit: how about calling this "leader_peer_uuids" or somesuch so it's clear that it's related to leader_uuid somehow. http://gerrit.cloudera.org:8080/#/c/14533/6/src/kudu/integration-tests/cluster_itest_util.cc@736 PS6, Line 736: : for (const auto& entry : tablet_servers) { : const auto& uuid = entry.first; : if (uuid != leader_uuid && ContainsKey(tablet_peer_uuids, uuid)) { : followers->emplace_back(entry.second); : } : } nit: How about iterating through `tablet_peer_uuids` and using FindOrDie() on `tablet_servers` (though skipping the leader)? FindOrDie should be guaranteed since we passed the check at L732. -- 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: 6 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 05:21:26 +0000 Gerrit-HasComments: Yes
