Alexey Serbin has posted comments on this change. Change subject: [security] load/store public TSK in the system table ......................................................................
Patch Set 13: (10 comments) > (4 comments) > > Now that I'm seeing the TSK deletion code alongside the insertion > code, I think it'd be a good idea to perform both operations in a > single sys catalog write. So the flow could look something like > this: > > <read all TSKs, keeping track of expired ones> > tsk_actions.tsks_to_delete = expired_TSKs > <check and generate new TSK> > if new TSK needed: > tsk_actions.tsks_to_add = new_TSK > Write(tsk_actions) > > What do you think? This sounds good from the performance perspective, but it introduces additional coupling. Also, imagine a situation when the delete ops of the combined request succeeds, but the inserts fail. Then we could end up in a situation when there is no TSK entries in the system table at all, and we have non-persisted key in run-time. The former is bad because the sequence of TSKs might be disrupted when leadership switches no another master instance (its TokenSigner will start with key sequence number 0 when there is TSK seq_num 100500 in the system. The latter is also not so good because the system could start using non-persisted TSK, and when leadership switched to another master, that valid key could no longer be seen. I would opt to keep it as is. 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: Line 22: #include <gflags/gflags_declare.h> > Nit: should precede gtest. Done PS13, Line 148: public part of TSK : // is persisted in the system catalog table > isn't the whole TSK persisted now? Done 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 l This is no longer needed as soon as the comment below is addressed. 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 Yes, it sounds reasonable to continue here in case of an error. For some reason I tried to preserve the same pattern as for the tablet metadata processing. PS13, Line 847: CHECK_OK(CheckGenerateNewTsk()); : CHECK_OK(DeleteTskEntries(expired_entry_ids)); > Agreed with Todd: the reason CHECK_OK() is reasonable on VisitTablesAndTabl Thank you for the clarification: now I understand the reasoning for CHECK_OK() usage at VisitTablesAndTablets() callsite. Yep, CheckInitAuthority() also does similar things. I'll update this accordingly. 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 wri It's a good observation. Line 3255: Status CatalogManager::CheckGenerateNewTsk() { > Should assert that the leader_lock_ is held here. Done PS13, Line 3281: set<string> ref(loader.expired_entry_ids()); : expired_entry_ids->swap(ref); > Can't this be combined? Unfortunately there is no assign for std::map, and inserting elements one-by-one is costly for tree maps. The latter variant does not look good to me: it implies you pass its constructor some sink where one of class methods writes. Does not smell good enough to me. Probably, using vectors instead of sets might be an option, but then it would be necessary to sort them if we want to remove duplicates. I would leave this as is for a while. 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 Done 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 cha I'm not sure I follow: you mean just print 8 bytes of 64-bit number as they are (in hexadecimal)? Yep, that would work as well. I thought having them in decimal would be easier for comprehension if ever looking at them. The idea was to have entry_id identifier which would allow ordered scans over those entries (in ascending order), but now it's not crucial. -- 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 <[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
