Alexey Serbin has posted comments on this change. Change subject: [c++ client] re-acquire authn token if expired ......................................................................
Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: Line 704: if (new_status.ok()) { > This can be kept the same as it was. Indeed -- it was better that way. 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(); > I meant a race between two threads with two separate KuduScanner instances Ah, I see. Sure, that could be a race, good point. I'll add a test for that. I'm curious why we don't see it now. There are other possible cases with concurrent writes and scans and also with concurrent client->master calls. I think sometime it resolves itself automatically (since server returns INVALID_AUTHENTICATION_TOKEN for absent authn token if the negotiation features at the client side were determined before token has been reset), and RuntimeError() for other cases is very unlikely. But it's necessary to fix that, I think. http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/client/scanner-internal.h File src/kudu/client/scanner-internal.h: PS8, Line 63: athentication > typo Done http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/integration-tests/authn_token_expire-itest.cc File src/kudu/integration-tests/authn_token_expire-itest.cc: PS8, Line 71: name_("k > typo Done http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/rpc/reactor.h File src/kudu/rpc/reactor.h: Line 185: std::unique_ptr<ErrorStatusPB> rpc_error); > Are these PBs wrapped in a unique_ptr just so they can be moved? That's fi Yes, partially. The main reason, though, is to enforce clear ownership semantics and avoid memory leaks. http://gerrit.cloudera.org:8080/#/c/6648/8/src/kudu/rpc/retriable_rpc.h File src/kudu/rpc/retriable_rpc.h: PS8, Line 92: autentication > typo Done PS8, Line 209: resp_.Clear(); : current_ = nullptr; > Are these two lines necessary for the token retry case as well? It's a good question. It seems the current_ does not need reset in that case (if the tablet server responded with INVALID_TOKEN it does not mean it's worth re-trying at other tserver. We can clean the prior response, but it seems it's not used anywhere in the INVALID_AUTHENTICATION_TOKEN code path. I'll clear it for uniformity. -- 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
