Alexey Serbin has posted comments on this change. Change subject: [c++ client] re-acquire authn token if expired ......................................................................
Patch Set 5: (15 comments) 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. Done 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 That's a good catch. Indeed, this method is called from other places as well. Line 706: const rpc::RpcController& controller(retrier().controller()); > this should be owned Done Line 708: const Status& controller_status = controller.status(); > this should be owned Done Line 709: if (controller_status.IsRemoteError()) { > Would it be possible to build this logic into the RpcRetrier instead of imp Good observation! I also was thinking about that. I think we can use RetriableRpc instead of this ad-hoc implementation and the implementation of KuduScanner. I think it's worth addressing that in a separate changelist. I'll file a Jira item for that. Line 769: if (new_status.IsNotAuthorized()) { > I don't think this is necessarily retriable. For instance it doesn't seem woops, that's a mistake -- it should contain all those check for RPC error code (I need to move that code from the RemoveError clause above). 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 There would be a race if any pair of methods like KuduScanner::{OpenTable(),NextRows()} were called on the same scanner object concurrently. Since the KuduScanner interface is explicitly declared non-thread-safe in client.h, those methods are not supposed to be called concurrently. 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 Done Line 186: TEST_F(AuthnTokenExpireITest, FetchNewAuthnTokenOnInsertAndScan) { > Is this testing something above and beyond InvalidTokenDuringWorkload? Per You are right. This test is obsolete once the Workload test appeared. I'll remove it. Sorry for this litter. Line 214: // Since the authn token is verified upon connection establishment, it's > Should this be handled by FLAGS_rpc_reopen_outbound_connections ? Done Line 234: TEST_F(AuthnTokenExpireITest, RestartTabletServers) { > I think this is a good test case, but I think FLAGS_rpc_reopen_outbound_con Absolutely. Good catch! Line 263: TEST_F(AuthnTokenExpireITest, RestartCluster) { > Likewise, I think FLAGS_rpc_reopen_outbound_connections shouldn't be set du I think you are absolutely right. These tests were written first, and then I added that re-open feature and the workload test, setting the flag everywhere. I'll fix 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. Done 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". Done Line 648: rthread->CompleteConnectionNegotiation(conn_, negotiation_status_, > while you are here, the negotiation_status_ should probably be moved as wel Done -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
