Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11448 )
Change subject: [security] test scenario for KUDU-2580 ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/11448/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11448/2//COMMIT_MSG@7 PS2, Line 7: secur > nit: I would categorize tests with the subsystem they test, so [security] i Done http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc File src/kudu/integration-tests/authn_token_expire-itest.cc: http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@106 PS2, Line 106: int num_tablet_servers) : : token_validity_seconds_(token_validi > Nit: if you're already changing the argument order, prefer num_masters befo Done http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@143 PS2, Line 143: /*num_tablet_servers=*/ 3) > nit: The prevailing way to format this is Done http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@144 PS2, Line 144: FATAL_INVALID_AUTH > Ditto. Done http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@360 PS2, Line 360: /*num_masters=*/ 1, : /*num_tablet_servers=*/ 3) { : cluster_opts_.extra_mast > Ditto. Done http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@425 PS2, Line 425: /*num_masters=*/ 3, : /*num_tablet_servers=*/ 3) { : > Ditto. Done http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@431 PS2, Line 431: // expiration of authn tokens, while the default authn expiration timeout : // is 7 days. So, let's make the token validity interval really short. : Substitute("--authn_token_validity_seconds=$0", token_validity_seconds_), : : // The default for leader_failure_max_missed_heartbeat_periods 3.0, but : // 2.0 is enough to have measter le > Can you justify these customized settings? Done http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@437 PS2, Line 437: // run a bit faster. > Was this just for debugging? Remove it now? Right, that's just some remnants from the debugging phase. http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@497 PS2, Line 497: : shared_ptr<KuduClient> client; > Unless you disable failure detection in the masters, how can you guarantee It's a good observation. With a small change it should be fine, and I'll update this correspondingly. The idea is to have a code where master re-election might mess up the test only to the point that this scenario will not give repro for KUDU-2580 and would not fail without the follow-up patch, but otherwise it should be safe -- it does not bring flakiness in terms of intermittent failures when the fix for KUDU-2580 is committed. http://gerrit.cloudera.org:8080/#/c/11448/2/src/kudu/integration-tests/authn_token_expire-itest.cc@519 PS2, Line 519: // but that would not induce errors in this cycle. The only negative outcome > Given that the whole thing is wrapped in ASSERT_EVENTUALLY, why do we need Good catch -- indeed, that is not needed. That's a stale piece from the initial approach. -- To view, visit http://gerrit.cloudera.org:8080/11448 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I58218deef24cca7c524bc61700cd400cdaabd050 Gerrit-Change-Number: 11448 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 19 Sep 2018 01:23:41 +0000 Gerrit-HasComments: Yes
