Dan Burkert has posted comments on this change. Change subject: [rpc] introduce per-RPC credentials policy ......................................................................
Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/6875/5/src/kudu/rpc/negotiation.cc File src/kudu/rpc/negotiation.cc: Line 215: conn->set_remote_features(client_negotiation.take_server_features()); I think we're being pessimistic here in a very certain case: The connection is created with CredentialsPolicy::ANY, but there currently is no token. In reality, that connection will end up using Kerberos if it's connecting to secure server. In that case, we could 'upgrade' ANY to PRIMARY. This could be done here by directly setting it on the connection, or you could store the negotiated AuthenticationType into the Connection instead of storing the CredentialsPolicy. I think this may be a little cleaner in the long run. http://gerrit.cloudera.org:8080/#/c/6875/5/src/kudu/rpc/reactor.cc File src/kudu/rpc/reactor.cc: PS5, Line 368: for (auto it = range.first; it != range.second; ++it) { : const auto& c = it->second; : if (c->scheduled_for_shutdown()) { : // This connection is not to be used for any new calls. : continue; : } : if (!c->SatisfiesCredentialsPolicy(cred_policy)) { : // Do not use a connection with a non-compliant credentials policy. : // Instead, open a new one, while marking the former as scheduled for : // shutdown. This process converges: any connection that satisfies the : // PRIMARY_CREDENTIALS policy automatically satisfies the ANY_CREDENTIALS : // policy as well. The idea is to keep only one usable connection identified : // by the specified 'conn_id'. : c->set_scheduled_for_shutdown(); : continue; : } : : // If the test-only 'one-connection-per-RPC' mode is enabled, connections : // are re-established at every RPC call. : if (PREDICT_FALSE(FLAGS_rpc_reopen_outbound_connections)) { : c->set_scheduled_for_shutdown(); : continue; : } : } : // Second pass: shutdown idle connections to the target destination if they : // have been marked. Non-idle connections will be taken care of later by the : // idle connection scanner. : scoped_refptr<Connection> found_conn; : for (auto it = range.first; it != range.second;) { : const auto& c = it->second; : if (c->scheduled_for_shutdown()) { : if (c->Idle()) { : DCHECK_EQ(Connection::CLIENT, c->direction()); : c->Shutdown(Status::NetworkError("connection is closed due to non-reuse policy")); : it = client_conns_.erase(it); : continue; : } : } else { : DCHECK(!found_conn); : found_conn = c; : // Appropriate connection is found; continue further to take care of the : // rest of connections marked for shutdown by the first pass above. : } : ++it; : } I think these loops can be simplified by combining them: scoped_refptr<Connection> found_conn; for (auto it = range.first; it != range.second;) { const auto& c = it->second; if (c->scheduled_for_shutdown() || !c->SatisfiesCredentialsPolicy(cred_policy) || PREDICT_FALSE(FLAGS_rpc_reopen_outbound_connections)) { // Shutdown idle connections. Non-idle connections will be taken care of // later by the idle connection scanner. if (c->Idle()) { DCHECK_EQ(Connection::CLIENT, c->direction()); c->Shutdown(Status::NetworkError("connection is closed due to non-reuse policy")); it = client_conns_.erase(it); continue; } c->set_scheduled_for_shutdown(); } else { DCHECK(!found_conn); found_conn = c; } ++it; } http://gerrit.cloudera.org:8080/#/c/6875/5/src/kudu/rpc/rpc-test.cc File src/kudu/rpc/rpc-test.cc: PS5, Line 295: PRC RPC -- 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: 5 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
