Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15671 )
Change subject: KUDU-3078 Refactor Sentry integration tests ...................................................................... Patch Set 4: (3 comments) http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/hms_itest-base.h File src/kudu/integration-tests/hms_itest-base.h: http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/hms_itest-base.h@28 PS4, Line 28: #include "kudu/integration-tests/external_mini_cluster-itest-base.h" // IWYU pragma: keep Why do we need this anymore? http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_hms-itest.cc@94 PS4, Line 94: return harness_.StartHms(cluster_); : } : : Status StopHms() { : return harness_.StopHms(cluster_); Why do these need to be internal to the harness? 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@414 PS2, Line 414: > Hm I'm not sure it would actually due to how this whole thing is templated Could you try to articulate what you dislike about the approach? Maybe post a WIP of your later patches that actually parameterize to use the harness. I'm finding it really difficult to understand how this harness will be used exactly, and that's making it difficult to understand why everything in this patch is needed. Taking a step back, I thought the goal of the harnesses was to let us rejigger existing Sentry tests to be Sentry/Ranger agnostic. In doing so, we could then templatize the tests with different harnesses. If that's true, it's surprising that the harnesses would have calls to StartHms/StopHms and StartSentry/StopSentry, given they are decidedly not Sentry/Ranger-agnostic. -- 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: 4 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: Wed, 08 Apr 2020 00:01:22 +0000 Gerrit-HasComments: Yes
