Alexey Serbin has posted comments on this change. Change subject: WIP [rpc] introduced per-RPC credentials policy ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/6875/2/src/kudu/rpc/reactor.cc File src/kudu/rpc/reactor.cc: Line 351: if (range.first != range.second) { > Is this if guard needed? Looks like it won't enter the loop anyway. You are right -- this scope is redundant. PS2, Line 361: con > typo: a connection with a non-compliant credentials policy thanks :) Line 376: // Test-only one-connection-per-RPC mode: try to re-establish connections. > Now that we have this schedule for shutdown idea, could we make the 'rpc_re It's a great idea. Done. Line 384: break; // No need to search futher; also, iterators are invalidated. > This seems brittle, could you instead continue looping and overwrite it wit Done http://gerrit.cloudera.org:8080/#/c/6875/2/src/kudu/rpc/reactor.h File src/kudu/rpc/reactor.h: Line 135: // Client-side connection map. > could you add a doc note here about why it's a multimap, maybe Done http://gerrit.cloudera.org:8080/#/c/6875/2/src/kudu/rpc/rpc_controller.h File src/kudu/rpc/rpc_controller.h: Line 58: ANY_BUT_AUTHN_TOKEN, > Perhaps this would be a good time to introduce a name for this concept of ' SGTM. http://gerrit.cloudera.org:8080/#/c/6875/2/src/kudu/rpc/user_credentials.h File src/kudu/rpc/user_credentials.h: Line 41: void set_authn_token(const AuthnToken& token); > I would have expected this to take a kudu::security::SignedTokenPB by value In some cases it might be necessary to clear the aggregated authn token, if it's set. To avoid using additional methods (like has_xxx(), reset_xxx()) I decided to use boost::optional . Probably, it's worth changing it to passing boost::optional by value? -- 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: 2 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
