[GitHub] [hbase] Apache9 commented on a change in pull request #2669: HBASE-25292 Improve InetSocketAddress usage discipline
Apache9 commented on a change in pull request #2669:
URL: https://github.com/apache/hbase/pull/2669#discussion_r530805655
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ConnectionUtils.java
##
@@ -128,32 +129,17 @@ public static void
setServerSideHConnectionRetriesConfig(final Configuration c,
}
/**
- * Return retires + 1. The returned value will be in range [1,
Integer.MAX_VALUE].
+ * Get a unique key for the rpc stub to the given server.
*/
- static int retries2Attempts(int retries) {
-return Math.max(1, retries == Integer.MAX_VALUE ? Integer.MAX_VALUE :
retries + 1);
+ static String getStubKey(String serviceName, ServerName serverName) {
Review comment:
As said before, I wonder what is the problem if we just use host:port
directly here? In the past I think the problem is that we will not resolve
again when connecting, for now, I think the problem has been solved?
##
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 callback) {
+ final Message param, Message returnType, final User ticket,
+ final InetSocketAddress inetAddr, final RpcCallback callback) {
Review comment:
Checked the code, I think we could avoid creating an InetSocketAddress
everytime here, we just need to change more classes to make use of Address
instead of InetSocketAddress, such as ConnetionId, FailedServers, as well as
the RpcClient interface. And the resolving of the actual address could be
delayed to NettyRpcConnection.connect and
BlockingRpcConnection.setupConnection, where we really want to connect to the
remote side. And once the connection has been established, and it has not been
closed because of error or idle for too long, we do not need to involve
InetSocketAddress again. I think it is OK?
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:
us...@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #2669: HBASE-25292 Improve InetSocketAddress usage discipline
Apache9 commented on a change in pull request #2669:
URL: https://github.com/apache/hbase/pull/2669#discussion_r526512744
##
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 callback) {
+ final Message param, Message returnType, final User ticket,
+ final InetSocketAddress inetAddr, final RpcCallback callback) {
Review comment:
Can not view the code now but IIRC, on this execution path, we will use
a ConnectionId to get a RpcConnection and then use it to send the rpc call?
Then I think we could put the actual resolving in the connect method? Before
connecting we could always use the Address class to represent the remote
address.
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:
us...@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #2669: HBASE-25292 Improve InetSocketAddress usage discipline
Apache9 commented on a change in pull request #2669:
URL: https://github.com/apache/hbase/pull/2669#discussion_r526510319
##
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'm not sure whether we really need to identify different region servers
here. What is the problem if we just use host and port as the stub key here? It
will not be a problem if we will resolve it when we actually connecting the
remote side? Thoughts?
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:
us...@infra.apache.org
[GitHub] [hbase] Apache9 commented on a change in pull request #2669: HBASE-25292 Improve InetSocketAddress usage discipline
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
concurrentCounterCache =
+ private static final LoadingCache
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 callback) {
+ final Message param, Message returnType, final User ticket,
+ final InetSocketAddress inetAddr, final RpcCallback 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:
us...@infra.apache.org
