Dan Burkert has posted comments on this change. Change subject: [c++ client] re-acquire authn token if expired ......................................................................
Patch Set 5: (16 comments) http://gerrit.cloudera.org:8080/#/c/6648/3//COMMIT_MSG Commit Message: PS3, Line 11: an a connection http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: Line 223: const Status& connect_status = ConnectToCluster(client, deadline); ConnectToCluster returns an owned Status. http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: Line 677: table_->client()->data_->messenger_->reset_authn_token(); Not every caller of this should be resetting the authn token. I think it's more appropriate to add this line in the block starting on line 713. Line 706: const rpc::RpcController& controller(retrier().controller()); this should be owned Line 708: const Status& controller_status = controller.status(); this should be owned Line 709: if (controller_status.IsRemoteError()) { Would it be possible to build this logic into the RpcRetrier instead of implementing it here? Line 769: if (new_status.IsNotAuthorized()) { I don't think this is necessarily retriable. For instance it doesn't seem to be retried in the scanner (it results in an OTHER_TS_ERROR) http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: Line 134: c->data_->messenger_->reset_authn_token(); Is there a race here between calling reset_authn_token() and another thread having already executed a successful ConnectToCluster, thus having already overwritten the expired token with a fresh one? http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/integration-tests/authn_token_expire-itest.cc File src/kudu/integration-tests/authn_token_expire-itest.cc: Line 105: std::copy(common_flags.begin(), common_flags.end(), This seems more difficult than directly inserting the flags into opts.extra_master_flags. Line 186: TEST_F(AuthnTokenExpireITest, FetchNewAuthnTokenOnInsertAndScan) { Is this testing something above and beyond InvalidTokenDuringWorkload? Perhaps they can be combined? Line 214: // Since the authn token is verified upon connection establishment, it's Should this be handled by FLAGS_rpc_reopen_outbound_connections ? Line 234: TEST_F(AuthnTokenExpireITest, RestartTabletServers) { I think this is a good test case, but I think FLAGS_rpc_reopen_outbound_connections shouldn't be set for it. Line 263: TEST_F(AuthnTokenExpireITest, RestartCluster) { Likewise, I think FLAGS_rpc_reopen_outbound_connections shouldn't be set during this. http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/rpc/client_negotiation.cc File src/kudu/rpc/client_negotiation.cc: Line 97: const string& code_name = ErrorStatusPB::RpcErrorCodePB_Name(error.code()); RpcErrorCodePB_Name returns an owned string. http://gerrit.cloudera.org:8080/#/c/6648/5/src/kudu/rpc/connection.cc File src/kudu/rpc/connection.cc: Line 148: LOG(WARNING) << "Shutting down connection " << ToString() the ToString already includes "connection". Line 648: rthread->CompleteConnectionNegotiation(conn_, negotiation_status_, while you are here, the negotiation_status_ should probably be moved as well. -- To view, visit http://gerrit.cloudera.org:8080/6648 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I418497ac59cfd4e476e9bfc6abe6b10b487712f5 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
