Dan Burkert 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. PS2, Line 361: con typo: a connection with a non-compliant credentials policy 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_reopen_outbound_connections' flag more powerful by marking the connection to be shutdown here and continuing? Line 384: break; // No need to search futher; also, iterators are invalidated. This seems brittle, could you instead continue looping and overwrite it with the result of the erase call? It's not an issue right now, but I think always doing a full loop-cycle will make this test-only codepath more similar to the real path. 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 // Client-side connection map. Multiple connections could be open to a remote server if multiple credential policies are used for individual RPCs. 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 'anything but authentication token'. Perhaps 'Primary Credentials'? Primary being Kerberos or cert, whereas authentication tokens would be 'Derived' or 'Secondary' credentials? 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, any reason to allow setting it to an empty optional? -- 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: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
