Todd Lipcon has posted comments on this change. Change subject: [security] load/store public TSK in the system table ......................................................................
Patch Set 13: (6 comments) http://gerrit.cloudera.org:8080/#/c/5935/13/src/kudu/integration-tests/token_signer-itest.cc File src/kudu/integration-tests/token_signer-itest.cc: PS13, Line 148: public part of TSK : // is persisted in the system catalog table isn't the whole TSK persisted now? http://gerrit.cloudera.org:8080/#/c/5935/13/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 438: do { hrm, I think it would be clearer to pull this inner block into a function like CatalogManagerBgTasks::RunOneIteration or something PS13, Line 454: LOG(ERROR) << "Error processing TSK entry, " : "aborting the current task: " << s.ToString(); : does this mean if we start getting a TSK-related error then we won't do any tablet processing? that seems like an unnecessary coupling, and we could instead just WARN and continue here on to doing the pending assignments, etc PS13, Line 847: CHECK_OK(CheckGenerateNewTsk()); : CHECK_OK(DeleteTskEntries(expired_entry_ids)); isn't it possible that we would lose leadership here? eg we propose the writes to the syscatalog and then they fail, wouldn't it be safe enough to just skip it? (under the assumption that we're about to become non-leader again) http://gerrit.cloudera.org:8080/#/c/5935/13/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: PS13, Line 522: et kEntryType http://gerrit.cloudera.org:8080/#/c/5935/13/src/kudu/master/sys_catalog.h File src/kudu/master/sys_catalog.h: PS13, Line 92: // Length of the TSK entry identifier in the system catalog: all keys have : // the same length and are string representation of a number in decimal : // notation padded with zeroes in the beginning. This is to have natural : // ordering of the records while doing scan of TSK entries. : static const int kSysTskEntryKeySize = 20; hrm, why not use big-endian int64? is this more convenient? (no need to change it, just curious) -- 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: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
