Adar Dembo 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 1: (28 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: class RetrieveAuthzTokenRpc : public AsyncLeaderMasterRpc<master::GetTableSchemaRequestPB, Doc the class too. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@87 PS1, Line 87: 'table_id' Not an argument. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@105 PS1, Line 105: // Authorization tokens stored for each table, indexed by the table id. Note Nit: empty line before. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.h@106 PS1, Line 106: users of the cache. Sentence fragment? 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@65 PS1, Line 65: RetrieveAuthzTokenRpc::RetrieveAuthzTokenRpc(const KuduTable* table, Why isn't the synchronous GetTableSchema method in client-internal.cc not built around this async primitive? http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@74 PS1, Line 74: req_.mutable_table()->set_table_name(table_->name()); : req_.mutable_table()->set_table_id(table_->id()); Shouldn't we only set one of these? Probably just the ID? http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@80 PS1, Line 80: AsyncLeaderMasterRpc:: Is this prefixing necessary? http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@85 PS1, Line 85: rpc.error_response()->unsupported_feature_flags_size() > 0) { This seems risky from a forward compatibility perspective: what if there are new feature flags that we do care about in the future? Is there a way to identify that it was just RETRIEVE_AUTHZ_TOKEN that was unsupported, and treat that as OK, while failing the rest? 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 already? http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@93 PS1, Line 93: auto* token_cache = client_->data_->authz_token_cache_.get(); Could be moved into the scope below, or even combined with L95. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@138 PS1, Line 138: EmplaceOrUpdate(&retrieve_authz_rpcs_, table_id, { rpc, { std::move(callback) } }); Not EmplaceOrDie here? Didn't we just prove that there's no such entry in retrieve_authz_rpcs_? http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/authz_token_cache.cc@151 PS1, Line 151: auto* rpc_and_cbs = FindOrNull(retrieve_authz_rpcs_, table_id); Would EraseKeyReturnValuePtr work here? It would be nice to find an alternative such that there aren't two lookups (oen in FindOrNull and one in erase). Maybe raw iterators are the way to go. 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: FALLTHROUGH_INTENDED; Nit: FWIW, I don't think this is necessary when the fallthrough is obvious (i.e. a case that does nothing). http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/batcher.cc@493 PS1, Line 493: if (!batcher_->client_->data_->FetchCachedAuthzToken(table()->id(), req_.mutable_authz_token())) { See my comment in scanner-internal.cc. 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: cahce cache http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5826 PS1, Line 5826: Parallelly Parallely? http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/client-test.cc@5852 PS1, Line 5852: ASSERT_LT(num_reqs, kThreads); Does this hold in TSAN environments with stress threads? In a pathological case it could be possible for each of the 20 threads to run serially and thus for there to be no grouping at all. Unlikely, but possible. 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: case ScanRpcStatus::RPC_INVALID_AUTHORIZATION_TOKEN: Nit: add a useful comment here, or drop the one from L143 if it's redundant. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc@352 PS1, Line 352: // Only new scan requests require authz tokens, with the expectation that : // further access to new scanner IDs will only be allowed for this user. I'm not really understanding the second half of this sentence. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/client/scanner-internal.cc@356 PS1, Line 356: // Note: this is expected if attempting to connect to a cluster that does : // not support fine-grained access control. Do we still want to have called mutable_authz_token() though? It means we'll send an empty token instead of not sending one at all. 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? SHARDS? 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: , Missing some value here? 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_ALLOW_SLOW_TESTS. http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc@167 PS1, Line 167: .timeout(MonoDelta::FromSeconds(60)) The default timeout value isn't appropriate? http://gerrit.cloudera.org:8080/#/c/12122/1/src/kudu/integration-tests/authz_token-itest.cc@263 PS1, Line 263: for (const auto& client_func : { insert_to_table, scan_from_table }) { Can you parameterize the test on these two? 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: // The master supports the 'RetrieveAuthzToken' RPC. I don't see RetrieveAuthzToken listed in MasterService. 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: void MasterServiceImpl::GetTableSchema(const GetTableSchemaRequestPB* req, Would be nice to "unit" test this bit of functionality in isolation, perhaps in master-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: const std::string& Can you use const char* here instead? -- 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: 1 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, 02 Jan 2019 21:27:01 +0000 Gerrit-HasComments: Yes
