Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12122 )
Change subject: KUDU-2543 pt 2: pass around default authz tokens ...................................................................... Patch Set 2: (33 comments) http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h File src/kudu/client/authz_token_cache.h: http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@47 PS1, Line 47: // An asynchonous RPC that retrieves a new authz token for a table and puts it > Doc the class too. Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@87 PS1, Line 87: se > Not an argument. Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@105 PS1, Line 105: const Status& status); > Nit: empty line before. Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@106 PS1, Line 106: > Sentence fragment? Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc File src/kudu/client/authz_token_cache.cc: http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@54 PS1, Line 54: using master::MasterFeatures; > warning: using decl 'SecureShortDebugString' is unused [misc-unused-using-d Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@65 PS1, Line 65: &MasterServiceProxy::GetTableSchemaAsync, "RetrieveAuthzToken", > Why isn't the synchronous GetTableSchema method in client-internal.cc not b This class is dependent on knowing the table ID up front. This is the case for all the places it's currently used, and isn't so for GetTableSchema. You're right that if we had the tablet ID, we could use this in GetTableSchema and forego the explicit Put() call in client-internal.cc http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@74 PS1, Line 74: void RetrieveAuthzTokenRpc::SendRpcCb(const Status& status) { : Status new_status = status; > Shouldn't we only set one of these? Probably just the ID? Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@80 PS1, Line 80: w_status = StatusFromP > Is this prefixing necessary? Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@85 PS1, Line 85: } > This seems risky from a forward compatibility perspective: what if there ar Yeah, this is actually not used. I added handling for this case in scanner-internal.cc and retriable_rpc.h (for WriteRpc), and changed the outcome -- if we are retrieving an authz token in response to the tserver requiring a new authz token, as we are here, the master must support authz tokens. I.e. the client should just spit out an error. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@92 PS1, Line 92: } > Do we need to do more error checking here? Or has that been done for us alr Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@93 PS1, Line 93: > Could be moved into the scope below, or even combined with L95. Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@138 PS1, Line 138: { > Not EmplaceOrDie here? Didn't we just prove that there's no such entry in r Done. It was having trouble deducing the type of the mapped Args for some reason, but this should work (calling the constructor explicitly). http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@151 PS1, Line 151: } // namespace client > Would EraseKeyReturnValuePtr work here? It would be nice to find an alterna Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/batcher.cc@384 PS1, Line 384: switch (err->code()) { > Nit: FWIW, I don't think this is necessary when the fallthrough is obvious Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/batcher.cc@493 PS1, Line 493: void WriteRpc::FetchCachedAuthzToken() { > See my comment in scanner-internal.cc. Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5799 PS1, Line 5799: data > cache Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5826 PS1, Line 5826: oken(table > Parallely? Self-conscious about this due to https://www.quora.com/Is-using-the-word-parallelly-grammatically-correct so changed the name. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5833 PS1, Line 5833: TEST_F(ClientTest, TestRetrieveAuthzTokenInParallel) { > warning: 'emplace_back' is called inside a loop; consider pre-allocating th Ack http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5852 PS1, Line 5852: // The authz token retrieval requests shouldn't send one request per table; > Does this hold in TSAN environments with stress threads? In a pathological Ran via dist-test with 28 stress threads with 0/1000 failures. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc@147 PS1, Line 147: reacquire_authn_token = true; > Nit: add a useful comment here, or drop the one from L143 if it's redundant Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc@352 PS1, Line 352: if (configuration().row_format_flags() & KuduScanner::PAD_UNIXTIME_MICROS_TO_16_BYTES) { : controller_.RequireServerFeature(TabletServerFeatures::PAD_UNIXTIME_MICR > I'm not really understanding the second half of this sentence. Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc@356 PS1, Line 356: // Only new scan requests require authz tokens. Scan continuations rely on : // Kudu's prevention of scanner hijacking by > Do we still want to have called mutable_authz_token() though? It means we'l Nice catch. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/CMakeLists.txt File src/kudu/integration-tests/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/CMakeLists.txt@61 PS1, Line 61: ADD_KUDU_TEST(authz_token-itest PROCESSORS 2) > PROCESSORS? SHARDS? We chatted a bit about this offline and I'm basing this off of that discussion. The test doesn't take anywhere near 900 seconds to run, so NUM_SHARDS is fine with the default. I'm going to base the PROCESSORS off of the value used by client-test, since they both use an IMC and don't do anything crazy with it. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authn_token_expire-itest.cc File src/kudu/integration-tests/authn_token_expire-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authn_token_expire-itest.cc@276 PS1, Line 276: V > Missing some value here? Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: PS1: > We should restrict tests that explicitly sleep for a second or more to KUUD Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc@94 PS1, Line 94: using security::PrivateKey; > warning: using decl 'SecureShortDebugString' is unused [misc-unused-using-d Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc@167 PS1, Line 167: > The default timeout value isn't appropriate? Done, pasted from client-test since these tests were there originally. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc@263 PS1, Line 263: // First, let's do a sanity check that initial authz tokens allow the user to > Can you parameterize the test on these two? Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master.proto@788 PS1, Line 788: // EVICT_FIRST (a.k.a. 3-2-3) and the PREPARE_REPLACEMENT_BEFORE_EVICTION > I don't see RetrieveAuthzToken listed in MasterService. Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/master/master_service.cc@404 PS1, Line 404: > Would be nice to "unit" test this bit of functionality in isolation, perhap Added a very basic test. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/rpc/retriable_rpc.h File src/kudu/rpc/retriable_rpc.h: http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/rpc/retriable_rpc.h@123 PS1, Line 123: > Can you use const char* here instead? Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/rpc/retriable_rpc.h@240 PS1, Line 240: return GetNewAuthnTokenAndRetry(); > warning: redundant boolean literal in conditional return statement [readabi Done http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/rpc/retriable_rpc.h@250 PS1, Line 250: if (server != nullptr && result.status.IsTimedOut()) { > warning: redundant boolean literal in conditional return statement [readabi Done -- To view, visit http://gerrit.cloudera.org:8080/12122 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7971d652d6adc822167cf959bffd5f994a7ca565 Gerrit-Change-Number: 12122 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 05 Jan 2019 00:40:26 +0000 Gerrit-HasComments: Yes
