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 10: (21 comments) http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h File src/kudu/client/authz_token_cache.h: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@47 PS10, Line 47: asynchonous > asynchronous Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@84 PS10, Line 84: // Adds an authz token to the cache for 'table_id', replacing any that : // previously existed. : void Put(const std::string& table_id, : security::SignedTokenPB authz_token); > Yep, indeed -- I took another look and found them there. Ack http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@123 PS10, Line 123: table id > nit: either 'table ID' or change 'table ID' --> 'table id' at line 130. Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc File src/kudu/client/authz_token_cache.cc: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@63 PS10, Line 63: > style nit: 4 spaces Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@84 PS10, Line 84: if (new_status.ok() && resp_.has_authz_token()) { : client_->data_->authz_token_cache_.Put(table_->id(), resp_.authz_token()); : } : user_cb_.Run(new_status); > What if (new_status.ok() && !resp_.has_authz_token())? Does that situation Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@73 PS10, Line 73: void RetrieveAuthzTokenRpc::SendRpcCb(const Status& status) { : Status new_status = status; : // Check for generic master RPC errors. : if (RetryOrReconnectIfNecessary(&new_status)) { : return; : } : // Unwrap and return any other application errors that may be returned by the : // master service. : if (new_status.ok() && resp_.has_error()) { : new_status = StatusFromPB(resp_.error().status()); : } : if (new_status.ok() && resp_.has_authz_token()) { : client_->data_->authz_token_cache_.Put(table_->id(), resp_.authz_token()); : } : user_cb_.Run(new_status); : } : : string RetrieveAuthzTokenRpc::ToString() const { : return Substitute("$0 { table: '$1' ($2), attempt: $3 }", AsyncLeaderMasterRpc::ToString(), : req_.table().table_name(), req_.table().table_id(), num_attempts()); : } > nit: could you reorder these to match the order of declaration in the .h fi Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@103 PS10, Line 103: std::lock_guard<simple_spinlock> l(token_lock_); > nit: would it make sense to use shared_lock pattern here? For example, tha I suppose, but the work done under lock is pretty paltry. I've generally used that pattern when there is more work to be done per reader or per writer; in this case, both readers and writers are just updating entries in a map. http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@123 PS10, Line 123: rpc_and_cbs->second.emplace_back(std::move(callback)); > nit: add DCHECK(!rpc_and_cbs->second.empty()) before adding a new element ( Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@129 PS10, Line 129: RpcAndCallbacks({ rpc, { std::move(callback) } }) > whoops, apparently you cannot Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@143 PS10, Line 143: rpc_and_cbs.second > I'm not sure .second is necessary here: the EraseKeyReturnValuePtr() is sup Right, but the mapped type is the RpcAndCallbacks pair. http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@143 PS10, Line 143: rpc_and_cbs.second > I take this back: I missed the fact the value in the map is std::pair. Ack http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client-test.cc@5833 PS10, Line 5833: ASSERT_TRUE(new_data->FetchCachedAuthzToken(table_id, &cached_token)); > Does it make sense to verify that storing authz token into 'new_data' does Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/client.h@637 PS10, Line 637: FRIEND_TEST(ClientTest, TestInvalidAuthorizationTokens); > Is it still around? For some reason, I could find this test in this change Indeed, I think these were roughly TestInvalidAuthzTokens. http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@143 PS10, Line 143: const char* kTableName = "test-table"; : const char* kUser = "token-user"; : const char* kBadUser = "bad-token-user"; > nit: 'const char* x const' protects the public pointer member 'x' itself. Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@207 PS10, Line 207: int > nit: the TotalCount() method of the metric returns uint64_t, why not to use Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@248 PS10, Line 248: with > drop Rewrote this snippet. http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@301 PS10, Line 301: char* byte = &bad_signature[i]; : *byte = ~*byte; > nit: I think it's possible to use less indirection-related operations here: Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@484 PS10, Line 484: keep : // failing > are always invalid ? Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/integration-tests/authz_token-itest.cc@540 PS10, Line 540: shared_ptr<KuduSession> session(client_->NewSession()); > Is this needed? Ah no, I refactored this into uselessness. http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master-test.cc@1771 PS10, Line 1771: ::testing::Values(true, false) > nit: could use ::testing::Bool() instead Done http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/master/master.proto@638 PS10, Line 638: A token can always be expected with this response > Right, so non-leaders will send back GetTableSchemaResponsePB with no authz Ah, you're right. If the response has an error, we shouldn't expect an authz token. -- 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: 10 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: Wed, 13 Feb 2019 00:45:48 +0000 Gerrit-HasComments: Yes
