Todd Lipcon has posted comments on this change. Change subject: [c++ client] re-acquire authn token if expired ......................................................................
Patch Set 16: (9 comments) http://gerrit.cloudera.org:8080/#/c/6648/16/docs/known_issues.adoc File docs/known_issues.adoc: PS16, Line 172: one week is now supported only for Kudu C++ client nit: I think for the context of limitations, it's better to be explicit about the negative cases. i.e "is not supported by the Java client" or "is only supported by the C++ client but not by the Java client" or somesuch. http://gerrit.cloudera.org:8080/#/c/6648/16/docs/security.adoc File docs/security.adoc: PS16, Line 221: Java nit: insert "The" PS16, Line 223: yet nit: we decided at some point to not use these time-related words in the docs, since they imply some future timeline, but we aren't actually talking about that timeline. PS16, Line 223: C++ nit: the PS16, Line 224: client clients http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: PS16, Line 415: existring typo http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: PS16, Line 731: ANY_BUT_AUTHN_OTKEN this isn't the current name (though I actually like something like you have here better!) http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: PS16, Line 165: rpc::CredentialsPolicy creds_policy = : rpc::CredentialsPolicy::ANY_CREDENTIALS); maybe it's just me, but I find this wrapping a bit odd. Maybe wrap all the params at the '(' instead so that the default param ends up further-indented from its param name? PS16, Line 237: scoped_refptr<internal::ConnectToClusterRpc> leader_master_rpc_any_creds_; : std::vector<StatusCallback> leader_master_callbacks_any_creds_; : scoped_refptr<internal::ConnectToClusterRpc> leader_master_rpc_primary_creds_; : std::vector<StatusCallback> leader_master_callbacks_primary_creds_; is it really necessary to split the callbacks? eg if you try to request a ConnectToCluster with primary-creds-only policy, but one is already running without that policy, the worst that happens is that the callback happens and, on your retry, you still don't have active tokens, and you have to reconnect again, right? i.e this could only be transient and only be transient once in a rare case -- 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: 16 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: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
