bbeaudreault commented on code in PR #5688:
URL: https://github.com/apache/hbase/pull/5688#discussion_r1501153941
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java:
##########
@@ -201,23 +209,31 @@ private void failInit(Channel ch, IOException e) {
// fail all pending calls
ch.pipeline().fireUserEventTriggered(BufferCallEvent.fail(e));
shutdown0();
+ rpcClient.failedServers.addToFailedServers(remoteId.getAddress(), e);
+ }
+
+ private void saslFailInit(Channel ch, String serverPrincipal, IOException
error) {
+ assert eventLoop.inEventLoop();
+ saslNegotiationDone(serverPrincipal, false);
+ failInit(ch, error);
}
- private void saslNegotiate(final Channel ch) {
+ private void saslNegotiate(Channel ch, String serverPrincipal) {
assert eventLoop.inEventLoop();
+ NettyFutureUtils.safeWriteAndFlush(ch,
connectionHeaderPreamble.retainedDuplicate());
Review Comment:
this also seems new, is there a reason?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/Call.java:
##########
@@ -85,16 +84,15 @@ class Call {
* Builds a simplified {@link #toString()} that includes just the id and
method name.
*/
public String toShortString() {
+ // Call[id=32153218,methodName=Get]
return new ToStringBuilder(this,
ToStringStyle.SHORT_PREFIX_STYLE).append("id", id)
- .append("methodName", md.getName()).toString();
+ .append("methodName", md != null ? md.getName() : "").toString();
Review Comment:
might be weird to see `Call[id=23123123,methodName=]` in null case. Is there
anything more useful we can put?
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java:
##########
@@ -201,23 +209,31 @@ private void failInit(Channel ch, IOException e) {
// fail all pending calls
ch.pipeline().fireUserEventTriggered(BufferCallEvent.fail(e));
shutdown0();
+ rpcClient.failedServers.addToFailedServers(remoteId.getAddress(), e);
Review Comment:
this is new right? what's the reason. it's probably good, but just checking
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java:
##########
@@ -201,23 +209,31 @@ private void failInit(Channel ch, IOException e) {
// fail all pending calls
ch.pipeline().fireUserEventTriggered(BufferCallEvent.fail(e));
shutdown0();
+ rpcClient.failedServers.addToFailedServers(remoteId.getAddress(), e);
+ }
+
+ private void saslFailInit(Channel ch, String serverPrincipal, IOException
error) {
+ assert eventLoop.inEventLoop();
+ saslNegotiationDone(serverPrincipal, false);
+ failInit(ch, error);
}
- private void saslNegotiate(final Channel ch) {
+ private void saslNegotiate(Channel ch, String serverPrincipal) {
assert eventLoop.inEventLoop();
+ NettyFutureUtils.safeWriteAndFlush(ch,
connectionHeaderPreamble.retainedDuplicate());
Review Comment:
nevermind i see this was added here from below
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java:
##########
@@ -252,35 +268,99 @@ public void operationComplete(Future<Boolean> future)
throws Exception {
p.remove(NettyHBaseRpcConnectionHeaderHandler.class);
// don't send connection header,
NettyHBaseRpcConnectionHeaderHandler
// sent it already
- established(ch);
+ saslEstablished(ch, serverPrincipal);
} else {
final Throwable error = future.cause();
scheduleRelogin(error);
- failInit(ch, toIOE(error));
+ saslFailInit(ch, serverPrincipal, toIOE(error));
}
}
});
} else {
// send the connection header to server
ch.write(connectionHeaderWithLength.retainedDuplicate());
- established(ch);
+ saslEstablished(ch, serverPrincipal);
}
} else {
final Throwable error = future.cause();
scheduleRelogin(error);
- failInit(ch, toIOE(error));
+ saslFailInit(ch, serverPrincipal, toIOE(error));
}
}
});
}
- private void getConnectionRegistry(Channel ch) throws IOException {
- established(ch);
- NettyFutureUtils.safeWriteAndFlush(ch,
- Unpooled.directBuffer(6).writeBytes(RpcClient.REGISTRY_PREAMBLE_HEADER));
+ private void getConnectionRegistry(Channel ch, Call connectionRegistryCall)
throws IOException {
+ assert eventLoop.inEventLoop();
+ PreambleCallHandler.setup(ch.pipeline(), rpcClient.readTO, this,
+ RpcClient.REGISTRY_PREAMBLE_HEADER, connectionRegistryCall);
}
- private void connect() throws UnknownHostException {
+ private void onSecurityPreambleError(Channel ch, Set<String>
serverPrincipals,
+ IOException error) {
+ assert eventLoop.inEventLoop();
+ LOG.debug("Error when trying to do a security preamble call to {}",
remoteId.address, error);
+ if (ConnectionUtils.isUnexpectedPreambleHeaderException(error)) {
+ // this means we are connecting to an old server which does not support
the security
+ // preamble call, so we should fallback to randomly select a principal
to use
+ // TODO: find a way to reconnect without failing all the pending calls,
for now, when we
+ // reach here, shutdown should have already been scheduled
+ return;
Review Comment:
Where does this fallback to random happen? I had the same question when
reading the blocking client impl.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
