Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15671 )
Change subject: KUDU-3078 Refactor Sentry integration tests ...................................................................... Patch Set 5: (21 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/mini-cluster/external_mini_cluster.h" > Why do we need this anymore? Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/hms_itest-base.h@92 PS4, Line 92: > warning: the parameter 'hms_opts' is copied for each invocation but only us Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/hms_itest-base.h@93 PS4, Line 93: hms::HmsClient* hms_client() { > warning: passing result of std::move() as a const reference argument; no mo Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/hms_itest-base.cc File src/kudu/integration-tests/hms_itest-base.cc: http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/hms_itest-base.cc@78 PS4, Line 78: Status HmsITestHarness::CreateKuduTable(const string& database_name, > warning: method 'CreateKuduTable' can be made static [readability-convert-m Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/hms_itest-base.cc@121 PS4, Line 121: const boost::optional<const string&>& kudu_table_name) { > warning: the parameter 'kudu_table_name' is copied for each invocation but Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/hms_itest-base.cc@186 PS4, Line 186: const boost::optional<const string&>& user, > warning: the parameter 'user' is copied for each invocation but only used a Done 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: } : : Status StopHms() { : return harness_.StopHms(cluster_); : } > Why do these need to be internal to the harness? I thought it's best to wrap everything for consistency. Do you think I should remove these? http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_hms-itest.cc@114 PS4, Line 114: return harness_.CreateHmsTable(database_name, table_name, table_type, kudu_table_name); > warning: the parameter 'kudu_table_name' is copied for each invocation but Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_hms-itest.cc@136 PS4, Line 136: const std::string& table_type = hms::HmsClient::kManagedTable) { > warning: the parameter 'user' is copied for each invocation but only used a Done 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: > Could you try to articulate what you dislike about the approach? Posted the next patch and added you as a reviewer. The problem with creating the opts your way is that --sentry_privileges_cache_capacity_mb=$0 is always to opts in case of Sentry with a different $0, so I would need to create the whole opts instead of extending the existing opts from the parent. I was actually referring to my approach that I don't like, not yours. I just didn't have time to figure out a nice way to solve this, I've been chasing bugs and trying to make this whole thing work before branching. http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc File src/kudu/integration-tests/master_sentry-itest.cc: http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc@170 PS4, Line 170: static Status GetTableLocationsWithTableId(const string& table_name, > warning: method 'GetTableLocationsWithTableId' can be made static [readabil Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc@171 PS4, Line 171: const optional<const string&>& table_id, > warning: the parameter 'table_id' is copied for each invocation but only us Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc@212 PS4, Line 212: const shared_ptr<KuduClient>& client) { > warning: the const qualified parameter 'client' is copied for each invocati Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc@220 PS4, Line 220: static Status IsCreateTableDone(const OperationParams& p, > warning: method 'IsCreateTableDone' can be made static [readability-convert Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc@226 PS4, Line 226: static Status DropTable(const OperationParams& p, > warning: method 'DropTable' can be made static [readability-convert-member- Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc@231 PS4, Line 231: static Status AlterTable(const OperationParams& p, > warning: method 'AlterTable' can be made static [readability-convert-member Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc@238 PS4, Line 238: static Status IsAlterTableDone(const OperationParams& p, > warning: method 'IsAlterTableDone' can be made static [readability-convert- Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc@244 PS4, Line 244: static Status RenameTable(const OperationParams& p, > warning: method 'RenameTable' can be made static [readability-convert-membe Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc@251 PS4, Line 251: static Status GetTableSchema(const OperationParams& p, > warning: method 'GetTableSchema' can be made static [readability-convert-me Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc@262 PS4, Line 262: static Status GetTabletLocations(const OperationParams& p, > warning: method 'GetTabletLocations' can be made static [readability-conver Done http://gerrit.cloudera.org:8080/#/c/15671/4/src/kudu/integration-tests/master_sentry-itest.cc@296 PS4, Line 296: ExternalMiniClusterOptions)>& cluster_start_func) { > warning: the parameter 'cluster_start_func' is copied for each invocation b 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: 5 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:34:14 +0000 Gerrit-HasComments: Yes
