Adar Dembo has posted comments on this change.
Change subject: [security] load/store public TSK in the system table
......................................................................
Patch Set 13:
(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?
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.
http://gerrit.cloudera.org:8080/#/c/5935/13/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
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
Agreed with Todd: the reason CHECK_OK() is reasonable on
VisitTablesAndTabletsUnlocked() is because that's a read-only operation, so
even if this master loses leadership, it should succeed. But these operations
(and CheckInitCertAuthority() too, right?) may write, and write failures
shouldn't crash the master.
Line 3255: Status CatalogManager::CheckGenerateNewTsk() {
Should assert that the leader_lock_ is held here.
PS13, Line 3281: set<string> ref(loader.expired_entry_ids());
: expired_entry_ids->swap(ref);
Can't this be combined?
expired_entry_ids->assign(loader.expired_entry_ids().begin(),
loader.expired_entry_ids().end());
Something like that?
Alternatively, you can declare an empty set locally, pass it by pointer to
TskEntryLoader, call VisitTskEntries(), and swap the set directly with
expired_entry_ids.
--
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
