HBASE-25292 Improve InetSocketAddress usage discipline #2669
Conversation
| this.socket.bind(this.rpcClient.localAddr); | ||
| } | ||
| NetUtils.connect(this.socket, remoteId.getAddress(), this.rpcClient.connectTO); | ||
| if (this.rpcClient.metrics != null) { |
There was a problem hiding this comment.
This change is a forward port of the branch-1 change here: https://github.com/apache/hbase/pull/2671/files#diff-1a7ec27a8107293b6c87132823c262fc250570687a40f45646c43ae46dc6b04eR255
This change fixes a bug we encountered in production while running in Amazon's Elastic Kubernetes Service.
| assert eventLoop.inEventLoop(); | ||
| LOG.trace("Connecting to {}", remoteId.address); | ||
|
|
||
| LOG.trace("Connecting to {}", remoteId.getAddress()); |
There was a problem hiding this comment.
This is the netty version of the bug fix here: https://github.com/apache/hbase/pull/2669/files#diff-1a7ec27a8107293b6c87132823c262fc250570687a40f45646c43ae46dc6b04eR259
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
The patch is big and I do not have enough time to fully review it. Will go out for a business trip until Friday so I think I need to deal with HBASE-18070 first.
In general I'm a big +1 on the approach here. Let's use our Addresss as much as possible in our code.
The creation of InetSocketAddress will introduce a dns lookup which could be very slow, and once it is created it will never resolve again, and if you use InetSocketAddress.createUnresolved, you will get trouble if you forget to resolve it when passing to connection related methods. Glad to see that we plan to improve it.
Thanks.
| // 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 |
There was a problem hiding this 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?
There was a problem hiding this comment.
So we should use the ServerName here directly to make the key instead?
I can do that. Let me make the change.
There was a problem hiding this 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?
| private int maxConcurrentCallsPerServer; | ||
|
|
||
| private static final LoadingCache<InetSocketAddress, AtomicInteger> concurrentCounterCache = | ||
| private static final LoadingCache<Address, AtomicInteger> concurrentCounterCache = |
There was a problem hiding this 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.
| 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) { |
There was a problem hiding this 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.
There was a problem hiding this comment.
Let me check.
What we want to avoid is making an ISA for every Call.
Will try this and get back to you.
There was a problem hiding this comment.
I can't do this or else there will be a new InetSocketAddress() for every callMethod(), which may cause a DNS lookup per call.
There was a problem hiding this comment.
Ok, done. Lookup done upon first call and then cached there; or else an exception or failure indication is returned at that time.
There was a problem hiding this 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.
There was a problem hiding this 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?
| final User ticket; | ||
| final String serviceName; | ||
| final InetSocketAddress address; | ||
| final Address address; |
|
In the description I said this:
Based on positive feedback from @Apache9 on the overall approach I will be more aggressive in replacing ISA with Address, for bind() cases too. It will be a separate commit on the PR branch so can be undone if there is a later concern. |
apurtell
force-pushed
the
HBASE-25292
branch
3 times, most recently
from
09b6eeb to
eaece7e
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
|
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
Overall looks positive, using Address seems quite better choice than caching InetSocketAddress. I am still trying to understand some code paths (specifically the ones related to RpcConnection) better.
| import java.net.InetAddress; | ||
| import java.net.InetSocketAddress; |
There was a problem hiding this comment.
nit: both InetAddress and InetSocketAddress are no longer in use, imports can be removed.
There was a problem hiding this comment.
Ok. There are going to be other checkstyle nits too, will look at the report and fix them all.
| import java.net.InetAddress; | ||
| import java.net.InetSocketAddress; |
There was a problem hiding this comment.
nit: same here, no longer in use.
| @Override | ||
| public InetSocketAddress[] getFavoredNodesForRegion(String encodedRegionName) { | ||
| return regionFavoredNodesMap.get(encodedRegionName); | ||
| return Address.toSocketAddress(regionFavoredNodesMap.get(encodedRegionName)); |
There was a problem hiding this comment.
This conversion to InetSocketAddress[] takes place for each new StoreFileWriter creation right? Is there any other usecase that I am missing here?
There was a problem hiding this comment.
Where do we check if these addresses are resolved? Or we don't need to for this specific use-case?
There was a problem hiding this comment.
This will happen whenever the regionserver needs to build a list of ISA of datanodes for handing to FileSystem#create, but only if Favored Nodes is enabled. (As far as I know, nobody actually uses favored nodes, well, perhaps Francis and ex-Yahoo team.) So yeah, just before store file creation time. We can hand the ISA directly to HDFS without checking ISA#isUnresolved because should an ISA be unresolved when HDFS tries to use it the Java network API will throw an exception, which will propagate up to us. However if you would prefer to check the resolution status of the ISAs and explicitly throw our own exception, the place to do this would be FSUtils.java where the call to FileSystem#create including ISA parameters is performed via reflection.
There was a problem hiding this comment.
the place to do this would be FSUtils.java where the call to FileSystem#create including ISA parameters is performed via reflection.
Oh, what a giant reflection happening here :)
return (FSDataOutputStream) (DistributedFileSystem.class
.getDeclaredMethod("create", Path.class, FsPermission.class, boolean.class, int.class,
short.class, long.class, Progressable.class, InetSocketAddress[].class)
.invoke(backingFs, path, perm, true, CommonFSUtils.getDefaultBufferSize(backingFs),
replication > 0 ? replication : CommonFSUtils.getDefaultReplication(backingFs, path),
CommonFSUtils.getDefaultBlockSize(backingFs, path), null, favoredNodes));
We can hand the ISA directly to HDFS without checking ISA#isUnresolved because should an ISA be unresolved when HDFS tries to use it the Java network API will throw an exception, which will propagate up to us.
Makes sense, I think we should be good with this as is, rather than performing resolution checks at both layers.
Yeah, this test is going to have to change or maybe go away. I will look at it. |
|
I decided not to try replacing ISA with Address for bind() use cases. The problem there is it leads to a lot of conversions between Address and ISA, back and forth. Frameworks like Java networking and Netty have methods that receive and return ISA. Whenever we call them, we have to make a conversion. An ISA created to represent a local identity for bind() does not have the same issues with caching that an ISA created to represent a remote identity for connect(). The local network identity is not expected to change over the lifetime of the process. If the container, VM, or server instance is replaced with a new instance with a new network identity, all currently running processes can be expected to be terminated as part of that action. I am not contemplating further changes on this PR beyond fixes needed for checkstyle, findbugs, or unit test findings. That said, if there is strong opinion that we should continue the replacement of ISA with Address for the bind() code paths, I will certainly do that for you. Be advised there will be a lot of conversions back and forth (akin to the many conversions between byte[] and String that bedevil us sometimes). |
apurtell
force-pushed
the
HBASE-25292
branch
from
e02560a to
42a9227
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Agree that bind is not important here. One of most the important things we want to fix here is to not cache the resolved address since it could be changed, especially in a container environment. But for bind, usually we will just bind once when starting a service, which means we do not have a chance to resolve it again. And I'm interest that you have already deployed HBase on K8S environment? Actually I'm running the K8s team at Xiaomi now(the HBase team is running by Guanghao now). If it is OK, would you mind to have an online technical talk with the K8s and HBase team at Xiaomi to talk about your solution? Thanks. |
apurtell
force-pushed
the
HBASE-25292
branch
2 times, most recently
from
e33cf77 to
14c3626
Compare
liuml07
left a comment
liuml07
left a comment
There was a problem hiding this comment.
I think I reviewed the similar code internally and was +1 on it.
I see some differences here in the patch for master branch, the code still looks good to me.
Thanks.
Thanks @apurtell. Our lead in HBase on Kubernetes is @dhirajh He was willing to share our experience in a blog post or talk, and I'm not sure if materials is ready. We can also schedule a meeting sometime after holiday season (early Dec or next year) if mutual interest is there. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
A blog post first will be good. We could read the blog post first to find out the things we have interest and want to know more details, then we could set up the topic to discuss in the meeting, which could make the meeting more efficient. And on the time of the meeting, no hurry. Take your time. And thanks all for your offers. We could discuss this further with email as this is not related to this PR. |
|
And on the PR, @apurtell please give me sometime to review. I want to take a deep look at the client side code. If you think I'm too late just ping me, and if I still do not have time to finish the review then you are free to merge. Thanks. |
|
🎊 +1 overall
This message was automatically generated. |
|
There is plenty of time for review, no worries. Will wait. |
Thanks for the interest @Apache9 and thanks @apurtell @liuml07 for adding me to the conversation. Sounds good on the plan, the blog post is not yet ready, but will work on getting that out. I am happy to participate in any meetings after that. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Network identities should be bound late. Remote addresses should be resolved at the last possible moment, just before connect(). Network identity mappings can change, so our code should not inappropriately cache them. Otherwise we might miss a change and fail to operate normally. Revert "HBASE-14544 Allow HConnectionImpl to not refresh the dns on errors" Removes hbase.resolve.hostnames.on.failure and related code. We always resolve hostnames, as late as possible. Preserve InetSocketAddress caching per RPC connection. Avoids potential lookups per Call. Replace InetSocketAddress with Address where used as a map key. If we want to key by hostname and/or resolved address we should be explicit about it. Using Address chooses mapping by hostname and port only. Add metrics for potential nameservice resolution attempts, whenever an InetSocketAddress is instantiated for connect; and metrics for failed resolution, whenever InetSocketAddress#isUnresolved on the new instance is true.
We resolve DNS at the latest possible time, at first call, and do not resolve hostnames for creating stubs at all, so this unit test cannot work now.
apurtell
force-pushed
the
HBASE-25292
branch
from
14c3626 to
df43787
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
left a comment
Apache9
left a comment
There was a problem hiding this comment.
I think what I mentioned about the rpc client related changes could be done as follow ons, so I plan to approve and merge this PR first.
But for the PR for branch-2, I suggest that we merge it after cutting branch-2.4, as without the follow ons I think the improvement is only half done, and it will be a pain to carry a half done improvement through a whole minor release line.
Thanks.
| */ | ||
| 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) { |
There was a problem hiding this 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?
| 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) { |
There was a problem hiding this 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?
|
Thank for for the merge @Apache9 . Let me backport to branch-2 and branch-1 now. |
Network identities should be bound late. Remote addresses should be resolved at the last possible moment, just before connect(). Network identity mappings can change, so our code should not inappropriately cache them. Otherwise we might miss a change and fail to operate normally. Revert "HBASE-14544 Allow HConnectionImpl to not refresh the dns on errors" Removes hbase.resolve.hostnames.on.failure and related code. We always resolve hostnames, as late as possible. Preserve InetSocketAddress caching per RPC connection. Avoids potential lookups per Call. Replace InetSocketAddress with Address where used as a map key. If we want to key by hostname and/or resolved address we should be explicit about it. Using Address chooses mapping by hostname and port only. Add metrics for potential nameservice resolution attempts, whenever an InetSocketAddress is instantiated for connect; and metrics for failed resolution, whenever InetSocketAddress#isUnresolved on the new instance is true. * Use ServerName directly to build a stub key * Resolve and cache ISA on a RpcChannel as late as possible, at first call * Remove now invalid unit test TestCIBadHostname We resolve DNS at the latest possible time, at first call, and do not resolve hostnames for creating stubs at all, so this unit test cannot work now. Reviewed-by: Mingliang Liu <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
Network identities should be bound late. Remote addresses should be resolved at the last possible moment, just before connect(). Network identity mappings can change, so our code should not inappropriately cache them. Otherwise we might miss a change and fail to operate normally. Revert "HBASE-14544 Allow HConnectionImpl to not refresh the dns on errors" Removes hbase.resolve.hostnames.on.failure and related code. We always resolve hostnames, as late as possible. Preserve InetSocketAddress caching per RPC connection. Avoids potential lookups per Call. Replace InetSocketAddress with Address where used as a map key. If we want to key by hostname and/or resolved address we should be explicit about it. Using Address chooses mapping by hostname and port only. Add metrics for potential nameservice resolution attempts, whenever an InetSocketAddress is instantiated for connect; and metrics for failed resolution, whenever InetSocketAddress#isUnresolved on the new instance is true. * Use ServerName directly to build a stub key * Resolve and cache ISA on a RpcChannel as late as possible, at first call * Remove now invalid unit test TestCIBadHostname We resolve DNS at the latest possible time, at first call, and do not resolve hostnames for creating stubs at all, so this unit test cannot work now. Reviewed-by: Mingliang Liu <[email protected]> Signed-off-by: Duo Zhang <[email protected]>
6 participants
We sometimes cache InetSocketAddress in data structures in an attempt to optimize away potential nameservice (DNS) lookups. This is, in general, an anti-pattern, because once an InetSocketAddress is resolved, resolution is never attempted again. The ideal pattern for connect() is ISA instantiation just before the connect() call, with no reuse of the ISA instance. For bind() we presume the local identity won't change while the process is live so usage and caching can be relaxed in that case.
Network identities should be bound late. This means addresses should be resolved at the last possible moment. Also, network identity mappings can change, so our code should not inappropriately cache them; otherwise we might miss a change and fail to operate normally. This is especially important in public cloud and Kubernetes settings, where network identities will change as VM or container instances are replaced.
I have reviewed the code for InetSocketAddress usage and in my opinion sometimes we are caching ISA acceptably, and in other cases we are not.
Correct cases:
Incorrect cases that can be fixed:
Incorrect cases that cannot be fixed:
While in this area it is trivial to add new client connect metrics for number of potential nameservice lookups (whenever we instantiate an ISA) and number of failed nameservice lookups (if the instantiated ISA is unresolved).
While in this area I also noticed we often directly access a field in ConnectionId where there is also a getter, so good practice is to use the getter instead.