Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12877 )
Change subject: [sentry] enable sentry integration for master stress test ...................................................................... Patch Set 2: (16 comments) http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc File src/kudu/integration-tests/alter_table-randomized-test.cc: http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@106 PS1, Line 106: _SHU > nit: remove Done http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@106 PS1, Line 106: > Nit: to enable Done http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@108 PS1, Line 108: const vector<int32_t> kBlockSizes = {0, 2 * 1024 * 1024, > +1 for SentryMode or at least have boolean constants named bool SENTRY_DISA Done http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@108 PS1, Line 108: const vector<int32_t> kBlockSizes = {0, 2 * 1024 * 1024, > We should probably avoid conflating the two. A disabled HMS integration wou Done http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@108 PS1, Line 108: const vector<int32_t> kBlockSizes = {0, 2 * 1024 * 1024, > Not that it changes anything, but might be more obvious to refer to the sec I agree with Andrew and use a SentryMode enum for it. http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@126 PS1, Line 126: // we end up using quite a bit of disk space. So, we disable it. > Is there another test fixture we could leverage to avoid duplicating all of I added this to cluster_itest_util.cc which seems to be a central place for itest util functions. http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@126 PS1, Line 126: // we end up using quite a bit of disk space. So, we disable it. > +1 seems this is copied in three tests; probably worth sticking it somewher Done http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@140 PS1, Line 140: > Would Dababase privilege be enough for these new scenarios? If granting Database level privilege, we need to ensure to grant the privilege on all involved databases. So I found it is easier use server level privilege here. http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@176 PS1, Line 176: > nit: will it be able to infer the type if having just No, it doesn't seem to work. Though I can explicit specify the type, which I don't see why is better than the existing way. http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/alter_table-randomized-test.cc@178 PS1, Line 178: // NULLable columns. : static const int32_t kNullValue = 0xdeadbeef; : // We use this special value to denote default values. : // We ensure that we never insert or update to this value except when the : // > nit: extra lines Done http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc File src/kudu/integration-tests/master-stress-test.cc: http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@107 PS1, Line 107: using kudu::tools::LeaderMasterProxy; > warning: using decl 'tuple' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@117 PS1, Line 117: > Same comments, etc. Done http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@118 PS1, Line 118: const MonoDelta > I meant since restarting masters is the crucial point of those scenarios, I Thanks a lot for the explanation! I don't expect Kudu's client to retry when receives NetworkError() (when Sentry is unavailable). Because Kudu master connecting to Sentry as the client which already retry (thift/client.h) for transient errors due to short periods of unavailability. And how client should handled NetworkError() (when Sentry is unavailable) are covered in master_sentry-itest.cc. So I don't see we need to restart Sentry in this test. http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master-stress-test.cc@522 PS1, Line 522: : OverrideFlagForSlowTests("num_delete_table_threads", "5"); : OverrideFlagForSlowTests("num_replace_tablet_threads", "5"); : OverrideFlagForSlowTests("num_seconds_to_run", "30"); : : // This is for SyncRpc() calls using LeaderMasterProxy. : FLAGS_timeout_ms = kDefaultAdminTimeout.ToMilliseconds(); : > nit: will it compile with some the reduced option below Same as my other comment for alter_table-randomized-test.cc. http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master_failover-itest.cc File src/kudu/integration-tests/master_failover-itest.cc: http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master_failover-itest.cc@85 PS1, Line 85: namespace client { > Same comments here as in alter_table-randomized-test. Done http://gerrit.cloudera.org:8080/#/c/12877/1/src/kudu/integration-tests/master_failover-itest.cc@196 PS1, Line 196: // leader master has been paused. : TEST_P(MasterFailoverTest, TestCreateTableSync) { : const char* kTableName = "default.test_create_table_sync"; : : if (!AllowSlowTests()) { : LOG(INFO) << "This > nit: extra lines Done -- To view, visit http://gerrit.cloudera.org:8080/12877 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic48aa1bfd0947c645bb81137bb34e6cdfc088cf4 Gerrit-Change-Number: 12877 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 19 Apr 2019 04:52:33 +0000 Gerrit-HasComments: Yes
