Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9373 )
Change subject: KUDU-2319 follower masters should be able to verify authn tokens ...................................................................... Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/integration-tests/security-master-auth-itest.cc File src/kudu/integration-tests/security-master-auth-itest.cc: http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/integration-tests/security-master-auth-itest.cc@119 PS6, Line 119: ASSERT_LE(0, verifier.GetMaxKnownKeySequenceNumber()); Shouldn't this be ASSERT_LT? http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1032 PS6, Line 1032: LOG_WITH_PREFIX(INFO) << kDescription << ": success"; Consider adding the last key ID to this message. http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1040 PS6, Line 1040: static const auto kTskRotationInterval = 'static const' seems a little fishy here - does this pick up the non-default value if it's set via a flag? I don't see how it would. Seem better to just have it be a normal local variable, the overhead can't be very high. http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@1050 PS6, Line 1050: master_->messenger()->token_verifier().GetMaxKnownKeySequenceNumber() < 0 || : !last_tspk_run->Initialized() Seems like these first two clauses can be removed if you just initialize 'last_tspk_run' to 0, e.g. the unix epoch. http://gerrit.cloudera.org:8080/#/c/9373/6/src/kudu/master/catalog_manager.cc@3875 PS6, Line 3875: LOG_WITH_PREFIX(INFO) << "Read public part of TSK: " << key.key_seq_num(); This seems like it's going to be pretty chatty, maybe vlog? We're already INFO logging when the entire process is done, which seems good enough to me. -- To view, visit http://gerrit.cloudera.org:8080/9373 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idcc92dd4fae3d555af563d86634c07d3d06147a7 Gerrit-Change-Number: 9373 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Fri, 23 Feb 2018 18:42:52 +0000 Gerrit-HasComments: Yes
