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 <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: Wed, 13 Feb 2019 00:45:48 +0000
Gerrit-HasComments: Yes

Reply via email to