Adar Dembo has posted comments on this change. Change subject: [security] load/store public TSK in the system table ......................................................................
Patch Set 23: (5 comments) > Looks like we're triggering another shutdown deadlock like > https://issues.apache.org/jira/browse/KUDU-1863. Perhaps we should > just switch over the master replication itest to use External > MiniCluster instead of the internal one, to avoid having to deal > with the shutdown races. Back when I was doing more multi-master work, I also observed that master_replication-itest was the only multi-master test to use an internal MiniCluster, but I drew the opposite conclusion: I thought this difference in coverage was valuable (though at-times frustrating). I have some fears about switching it: 1. The timing of an internal MiniCluster is somewhat different than an external one, such that it can expose different races. 2. A working shutdown path itself may be useful if/when we implement "graceful shutdown" functionality, and if we lose multi-master shutdown coverage, it may continue to bitrot such that when we want to enable it, we'll face a much longer uphill climb. I'm open to being convinced, but if action is going to be taken, I suggest it is done outside of this (already long) code review. 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 (without sleep) may succeed? 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: if (ls.IsServiceUnavailable() { continue; } RETURN_NOT_OK(ls); Less nesting too. http://gerrit.cloudera.org:8080/#/c/5935/13/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS13, Line 3281: SysTskEntryPB sys_entry; : sys_entry.mutable_tsk()->Swap > Unfortunately there is no assign for std::map, and inserting elements one-b There's no std::map here, but your point is well-taken: there's no std::set::assign(). Would you find std::set::insert() more agreeable? I think there's a variant that allows range insertion, so you could do expired_entry_ids->insert(...begin(), ...end()). Could precede it with a expired_entry_ids->clear() if you're worried about the caller passing in a populated set. 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). Line 3293: leader_lock_.AssertAcquiredForReading(); > I know you and Adar talked about this a bit on Friday on slack, but this se Yes, we discussed it, but I think we came to two different conclusions. :) I originally suggested to Alexey to assert on the lock here, without noticing that it was called from both a read and a write context. When Alexey pointed that out, I thought something like RWMutex::AssertAcquiredForReadOrWrite wasn't worth the complexity, and suggested that Alexey just ignore my feedback. So, I'm not an advocate for this new approach, which I also think has a high "jarring to helping" ratio. I think dropping the assertion altogether is fine. AssertAcquiredForReadOrWrite can be done, but it's annoying to have to do it for such a minor use case. -- 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
