Hao Hao 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 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/authz_token_cache.h File src/kudu/client/authz_token_cache.h: http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/authz_token_cache.h@134 PS5, Line 134: retrieve_authz_rpcs_ nit: why not name it 'authz_rpcs_' to match 'authz_tokens_'? http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-internal.cc@466 PS5, Line 466: authz_token_cache_.Put nit: use StoreAuthzToken() instead? http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/client/client-test.cc@5853 PS5, Line 5853: client_->data_->RetrieveAuthzTokenAsync(client_table_.get(), s.AsStatusCallback(), Is it possible to change the logic to create a new client and only retrieve authz token when there isn't one cached? So that we can have a deterministic result that there is only one RPC being sent in this case? http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc File src/kudu/integration-tests/auth_token_expire-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc@143 PS5, Line 143: nit: extra space. http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/auth_token_expire-itest.cc@254 PS5, Line 254: nit: space. http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@184 PS5, Line 184: Insert nit: 'Inserts' to aligned with other comments style. http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@205 PS5, Line 205: Get nit: 'Gets' http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@281 PS5, Line 281: LOG(INFO) << "Trying to use the wrong user's token..."; Can you add a comment to explain what is the expected behavior in this case? Especially why operations in L285 is expected to succeed. http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@362 PS5, Line 362: ASSERT_OK(ScanFromTable(client_table_.get())); Why not parameterized the functor here as well? http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@369 PS5, Line 369: TestUnknownTsk Should a similar test be exist for authn token as well? http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@477 PS5, Line 477: TestMasterElectionStorms Will a similar also apply for authn tokens? Also, wondering how you consider what should be tested in auth_token_expire-itest and what should be in authz_token-itest? http://gerrit.cloudera.org:8080/#/c/12122/5/src/kudu/integration-tests/authz_token-itest.cc@523 PS5, Line 523: // Ensures the client can still communicate with servers that do not support : // authz tokens. What happens if a old client try to communicate with servers that require authz tokens? -- 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: 5 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, 09 Jan 2019 06:31:27 +0000 Gerrit-HasComments: Yes
