Alexey Serbin has posted comments on this change. Change subject: [security] load/store public TSK in the system table ......................................................................
Patch Set 23: (10 comments) http://gerrit.cloudera.org:8080/#/c/5935/23/src/kudu/integration-tests/registration-test.cc File src/kudu/integration-tests/registration-test.cc: Line 104: SleepFor(MonoDelta::FromMilliseconds(1)); > Why move this up? Can't we optimistically assume that the first call (witho It seemed to me the easiest way to amend the code to handle ServiceUnavailable. I agree it's not the best way. I'll fix that. PS23, Line 111: if (!ls.ok()) { : if (ls.IsServiceUnavailable()) { : // ServiceUnavailable means catalog manager is not yet ready : // to serve requests -- try again at a later time. : continue; : } : return ls; : } > FWIW, I find the following more idiomatic: That's much better, thanks. http://gerrit.cloudera.org:8080/#/c/5935/23/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS23, Line 212: using security::TokenSigner; : using security::TokenSigningPrivateKey; : using security::TokenSigningPrivateKeyPB; > Nit: sort order (with other using statements). Done Line 333: explicit TskEntryLoader(int64_t entry_expiration_seconds) > do we ever use anything here except for WallTime_Now()? probably better to Good idea. PS23, Line 452: } else { : if (l.leader_status().ok()) { > hm, why'd this change from else if to else { if { ? uh-oh -- that's just another artifact from my previous change (with do { ...} while() cycle) which has been rolled back. Good catch -- I will fix this. PS23, Line 839: if (!s.ok()) { : LOG(ERROR) << "Failed to intialize IPKI certificate info: " : << s.ToString(); : return; > I'm a little worried that, if one of these things fails, we'll return from I separated the loading and generation part. The interesting part of this process is that it's necessary to make sure the generated entries are written prior to loading those into memory. If doing so, I'm not quite sure it's safe to do so outside of the transition lock. Line 3293: leader_lock_.AssertAcquiredForReading(); > Yes, we discussed it, but I think we came to two different conclusions. :) Done Line 3293: leader_lock_.AssertAcquiredForReading(); > I know you and Adar talked about this a bit on Friday on slack, but this se All right, decided to drop the assertions. It looks ugly, indeed. Line 3293: leader_lock_.AssertAcquiredForReading(); > That's fair, I'm also fine with dropping it and relying on TSAN to check fo Done http://gerrit.cloudera.org:8080/#/c/5935/23/src/kudu/master/master.cc File src/kudu/master/master.cc: PS23, Line 121: mananger > nit: typo Done -- To view, visit http://gerrit.cloudera.org:8080/5935 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie91d4129bda0ca49e81988c28385895a2abcd201 Gerrit-PatchSet: 23 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
