Alexey Serbin 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 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc File src/kudu/integration-tests/cluster_itest_util.cc: http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@668 PS1, Line 668: const auto& const auto* ? http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@694 PS1, Line 694: DCHECK(!cstate.leader_uuid().empty()); In some intermittent states Raft consensus might have no leader. I think in that case this method can return Status::IllegalState(). >From the upper level, there should be a way to get eventual success from this >method using ASSERT_EVENTUALLY() or alike. http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/integration-tests/cluster_itest_util.cc@719 PS1, Line 719: DCHECK It would be great if this method worked the same way for both debug and release builds. So, maybe use turn this into 'return Status::InvalidArgument()'? http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@190 PS1, Line 190: TServerDetails * nit: in Kudu source we tend to stick the asterisk to the type http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@191 PS1, Line 191: MonoDelta::FromSeconds(30) Maybe, introduce a constant and use it here and at line 193 as well? http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@192 PS1, Line 192: TServerDetails * ditto http://gerrit.cloudera.org:8080/#/c/14533/1/src/kudu/tools/kudu-admin-test.cc@193 PS1, Line 193: tablet_servers_ Does it make sense to add a 'negative' test case when the set of supplied tablet servers doesn't contain the whole set of servers which host tablet replicas? -- 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: 1 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 23 Oct 2019 02:48:41 +0000 Gerrit-HasComments: Yes
