Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15671 )
Change subject: KUDU-3078 Refactor Sentry integration tests ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/hms_itest-base.h File src/kudu/integration-tests/hms_itest-base.h: http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/hms_itest-base.h@36 PS2, Line 36: onst std::unique_ptr<cluster > client::sp::shared_ptr<> is a shared_ptr template we expose for clients tha Done http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/hms_itest-base.h@87 PS2, Line 87: > Does this need to be public? Done http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/hms_itest-base.cc File src/kudu/integration-tests/hms_itest-base.cc: http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/hms_itest-base.cc@59 PS2, Line 59: Status HmsITestHarness::StopHms(const unique_ptr<cluster::ExternalMiniCluster>& cluster) { > nit: If this is why you're using shared_ptr everywhere, you can pass unique Done http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/master_hms-itest.cc@80 PS2, Line 80: hms_opts.enable_kerberos = EnableKerberos(); : hms_opts.service_principal = "hive"; : harness_.hms_client_.reset(new HmsC > nit: maybe add a getter for this. Also, rather than having the client be a hm maybe I'm missing something, but how would I reset hms_client_ if I access it via a getter? http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/master_sentry-itest.cc@292 PS2, Line 292: e fun > nit: maybe rename this to SetupCluster or somesuch? Just so readers don't c Done http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/master_sentry-itest.cc@293 PS2, Line 293: tialize the ExternalMiniCluster with the passed > nit: Should doc what this is expected to do, and maybe note that this might Done http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/master_sentry-itest.cc@382 PS2, Line 382: > nit: Why were these necessary? Done http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/master_sentry-itest.cc@414 PS2, Line 414: > nit: it might be simpler to just define another harness that's basically id maybe I misunderstood what you're suggesting, but wouldn't that be duplicating the whole setup? That way we would need to make sure to update two separate methods if we need to change anything in the setup code wouldn't we? http://gerrit.cloudera.org:8080/#/c/15671/2/src/kudu/integration-tests/master_sentry-itest.cc@413 PS2, Line 413: AddExtraOpts(&opts); : StartClusterWithOpts(opts); : return cluster_; : }); : > nit: spacing https://google.github.io/styleguide/cppguide.html#Formatting_L Done -- To view, visit http://gerrit.cloudera.org:8080/15671 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieff25eaf7a41cf7d8fe965738907e67bbd9b9051 Gerrit-Change-Number: 15671 Gerrit-PatchSet: 3 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 07 Apr 2020 20:40:48 +0000 Gerrit-HasComments: Yes
