Alexey Serbin 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: (12 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 http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.h@49 PS10, Line 49: class RetrieveAuthzTokenRpc : public AsyncLeaderMasterRpc<master::GetTableSchemaRequestPB, : master::GetTableSchemaResponsePB>, > I think it would be uglier, and since 100 is the strict limit, I'm OK with SGTM: it's just a nit, feel free to leave as is. 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); > Look again? They're both there. Yep, indeed -- I took another look and found them there. 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 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 need some special handling or at least DCHECK()? 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 file? 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, that might make sense if multiple sessions are run concurrently for the same instance of KuduClient. 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 (that would pair the DCHECK at line 145)? http://gerrit.cloudera.org:8080/#/c/12122/10/src/kudu/client/authz_token_cache.cc@129 PS10, Line 129: RpcAndCallbacks({ rpc, { std::move(callback) } }) nit: could you get away with just { rpc, { std::move(callback) } } ? 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 supposed to the return the mapped type, i.e. vector<StatusCallback>, right? 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@301 PS10, Line 301: char* byte = &bad_signature[i]; : *byte = ~*byte; nit: I think it's possible to use less indirection-related operations here: auto& byte = bad_signature[i]; byte = ~byte; 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 > Non-leader masters do not respond to GetTableSchema requests, see MasterSer Right, so non-leaders will send back GetTableSchemaResponsePB with no authz token, right? -- 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 <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 11 Feb 2019 02:35:24 +0000 Gerrit-HasComments: Yes
