Apache9 commented on a change in pull request #2669:
URL: https://github.com/apache/hbase/pull/2669#discussion_r525646192
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java
##########
@@ -135,10 +136,10 @@
private int maxConcurrentCallsPerServer;
- private static final LoadingCache<InetSocketAddress, AtomicInteger>
concurrentCounterCache =
+ private static final LoadingCache<Address, AtomicInteger>
concurrentCounterCache =
Review comment:
Good. When implementing an in-house rpc framework in the past, I used to
use InetSocketAddress.createUnresolved. But it has a problem that usually a
network framework will not accept a unresolved InetSocketAddress so if you
forget to recreate a resolved one you will get exception. Since here we have a
special structure, I think it is good to make use it to explicitly say that,
here we do not want a resolve yet.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -186,6 +183,33 @@ private void spawnRenewalChore(final UserGroupInformation
user) {
authService.scheduleChore(AuthUtil.getAuthRenewalChore(user));
}
+ /**
+ * Get a unique key for the rpc stub to the given server.
+ */
+ private String getStubKey(String serviceName, ServerName serverName) throws
UnknownHostException {
+ // Sometimes, servers go down and they come back up with the same hostname
but a different
+ // IP address. Force a resolution of the hostname by trying to instantiate
an
+ // InetSocketAddress, and this way we will rightfully get a new stubKey.
+ // Also, include the hostname in the key so as to take care of those cases
where the
+ // DNS name is different but IP address remains the same.
+ String hostname = serverName.getHostname();
+ int port = serverName.getPort();
+ // We used to ignore when the address was unresolvable but that makes no
sense. It
Review comment:
I do not think the old behavior is to ignore the unresolveable address?
It just wants to make the stub key shorter and do not need to actual do a DNS
lookup if we can make sure that the hostname will not change. And this is
important for an async implementation, as we do not expect this method to be
blocked but a DNS lookup could take several seconds if the the hostname can not
be resolved.
And in general, I never understand why here we need to add the ip address in
the stub key... We have timestamp in server name so we could know whether it is
the same region server, and for the rpc framework, there is no problem that
they have the same stub key? We just use a string here and once we want to
connect, we will resolve it and it will point to the correct ip address. We
could point the hostname of a regionserver to another regionserver while both
the regionservers are alive and can accept requests? This is not a good
practice and can cause big troubles... I guess once we have done this, the old
regionserver need to reconnect to master to again to tell master that its
hostname has been changed?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/ConnectionId.java
##########
@@ -32,9 +32,9 @@
private static final int PRIME = 16777619;
final User ticket;
final String serviceName;
- final InetSocketAddress address;
+ final Address address;
Review comment:
Good.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java
##########
@@ -390,8 +391,8 @@ private void onCallFinished(Call call, HBaseRpcController
hrc, InetSocketAddress
}
Call callMethod(final Descriptors.MethodDescriptor md, final
HBaseRpcController hrc,
- final Message param, Message returnType, final User ticket, final
InetSocketAddress addr,
- final RpcCallback<Message> callback) {
+ final Message param, Message returnType, final User ticket,
+ final InetSocketAddress inetAddr, final RpcCallback<Message> callback) {
Review comment:
Could we change this to use Address directly? And we could also remove
the UnknownHostException from the createAddr method then which could makes the
createRpcChannel and createBlockingRpcChannel not throw IOException, which will
be very good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
