You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/01/02 21:27:01 UTC
[kudu-CR] KUDU-2543 pt 2: pass around default authz tokens
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 <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 02 Jan 2019 21:27:01 +0000
Gerrit-HasComments: Yes