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 3: (8 comments) The test failure looks relevant. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc File src/kudu/client/authz_token_cache.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc@79 PS3, Line 79: if (new_status.ok() && resp_.has_error()) { So we don't need to check the controller status at all? Why not? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc@82 PS3, Line 82: if (resp_.has_authz_token()) { Shouldn't this also be conditioned on new_status.ok()? I wouldn't expect the server to return an error and for there to be a token in the response, but defensive programming... http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/authz_token_cache.cc@141 PS3, Line 141: auto rpc_and_cbs = EraseKeyReturnValuePtr(&retrieve_authz_rpcs_, table_id); What does this return if for some reason there's no key of table_id? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/batcher.cc@496 PS3, Line 496: *req_.mutable_authz_token() = signed_token; Can you std::move it? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/client/scanner-internal.cc@360 PS3, Line 360: *next_req_.mutable_new_scan_request()->mutable_authz_token() = authz_token; std::move http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc File src/kudu/integration-tests/authz_token-itest.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@153 PS3, Line 153: static void StoreAuthzToken(KuduClient* client, const string& table_id, const SignedTokenPB& token) { Line too long? http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/integration-tests/authz_token-itest.cc@347 PS3, Line 347: SKIP_IF_SLOW_NOT_ALLOWED(); Clever, more terse than the multi-line if check. http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/12122/3/src/kudu/master/master-test.cc@1752 PS3, Line 1752: for (bool supports_authz : { true, false }) { Could you parameterize the test on this? -- 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: 3 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: Sun, 06 Jan 2019 05:04:05 +0000 Gerrit-HasComments: Yes
