Alexey Serbin has posted comments on this change. Change subject: [rpc] introduce per-RPC credentials policy ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/6875/3//COMMIT_MSG Commit Message: Line 7: WIP [rpc] introduced per-RPC credentials policy > nit: better to say "Introduce" instead of "Introduced" for consistency with Done PS3, Line 23: but > other than Done http://gerrit.cloudera.org:8080/#/c/6875/3/src/kudu/rpc/reactor.cc File src/kudu/rpc/reactor.cc: Line 386: ++it; > Can you add a comment why not breaking here? Done PS3, Line 411: { : UserCredentials credentials; : credentials.set_real_user(conn_id.user_credentials().real_user()); : credentials.set_authn_token((policy == CredentialsPolicy::PRIMARY_CREDENTIALS) : ? boost::none : reactor()->messenger()->authn_token()); : (*conn)->set_local_user_credentials(std::move(credentials)); : } > There's a bit of a mess here, in that the Proxy object has set_user_credent Thank you for bringing this up. I had some doubts about this piece, and you clearly explained why it's not the right way to handling authn token in this context. Line 516: it = client_conns_.erase(it); > The assertion is preserved, but it changed its form into CHECK(range.first Actually, I just added 'break' here. There is no need to continue after erasing the necessary element. http://gerrit.cloudera.org:8080/#/c/6875/3/src/kudu/rpc/rpc_controller.h File src/kudu/rpc/rpc_controller.h: PS3, Line 51: on > nit: "on a" Done PS3, Line 59: username&password > we don't actually support username/password anymore, right? Right. That was more of an example. Agreed -- it's better to remove this for clarity. -- To view, visit http://gerrit.cloudera.org:8080/6875 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I52f806e7b6f6362f66148530124e748e199ae6c2 Gerrit-PatchSet: 3 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: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
