Alexey Serbin has posted comments on this change. Change subject: [c++ client] re-acquire authn token if expired ......................................................................
Patch Set 16: (12 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 abo Done http://gerrit.cloudera.org:8080/#/c/6648/16/docs/security.adoc File docs/security.adoc: PS16, Line 221: Java > nit: insert "The" Done PS16, Line 223: yet > nit: we decided at some point to not use these time-related words in the do Done PS16, Line 223: C++ > nit: the Done PS16, Line 224: client > clients Done http://gerrit.cloudera.org:8080/#/c/6648/16/src/kudu/client/batcher.cc File src/kudu/client/batcher.cc: PS16, Line 415: existring > typo Done 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 > I suggested the new name, so I'll go to bat for it. Besides being a positi ok. ANY_CREDENTIALS and PRIMARY_CREDENTIALS, right? Line 740: DCHECK(creds_policy == CredentialsPolicy::ANY_CREDENTIALS || > Isn't this statically verified by the compiler? I don't think it's going to be verified by compiler if somebody adds a new credentials policy, like SECONDARY_CREDENTIALS (meaning only secondary credentials may be used for connection negotiation). This debug-only assert will catch the case when this piece of code is not updated after that. Line 753: if (leader_master_rpc_primary_creds_ || need_new_rpc_primary_creds) { > I was having a hard time following this, so I tried to take a crack at redu To me -- nope, it's not :) I also spent some time trying to make sure that your code does what I want. It also has a misleading comment on non-existent connections if the final 'else' part. The two boolean flags store the fact whether it's necessary to create new RPC calls with appropriate credentials. Line 761: scoped_refptr<internal::ConnectToClusterRpc>& rpc( > This statement confusing. I think it's copying the scoped_refptr to an unn What would be that 'unnamed stack-allocated variable'? It just makes a reference either to leader_master_rpc_primary_creds_ or to leader_master_rpc_any_creds_ I'm not sure I understand: what unbound variable are you talking about? 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 I wrapped all parameters at +4 shift -- that looks cleaner (at least no additional line just for the default value). 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_; > Yes, the idea was to not wait for that re-connection again in that rare cas Also, if we don't split the callbacks, how do we know the process of scheduling the retries converges at all? I.e., consider the case when a retries are scheduled again and again, and the any-creds call always arrives first all the time, then comes the primary-creds call, and that so long? -- 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
