[HBASE-25292] Improve InetSocketAddress usage discipline - ASF JIRA

Details

Type: Bug
Status: Resolved
Priority: Major
Resolution: Fixed
Affects Version/s: None
Fix Version/s: 3.0.0-alpha-1, 1.7.0, 2.5.0, 2.4.18
Component/s: Client, HFile
Labels:
None

Hadoop Flags:

Reviewed

Description

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.

If I can restate my proposal for a usage convention for InetSocketAddress, it would be this: 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.

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:

We cache ISA for RPC connections, so we don't potentially do a lookup for every Call. However, we resolve the address earlier than we need to. The code can be improved by moving resolution to just before where we connect().
Use of ISA with bind. Typical uses like bindAddress, listenerAddress, initialIsa, or localAddress.
- (There is no harm to keep direct use of ISA for bind() but these could all be replaced with Address and on-demand create of ISA just before bind().
ClusterStatusPublisher in master.
Netty integration. Netty accepts and supplies ISA, no choice there, but we want to resolve at channel create time and cache for lifetime of the channel anyway. If a remote host goes away and is replaced with a new identity, a new channel/connection will be created with a new resolution just before connect() via higher layer error handling.

Incorrect cases that can be fixed:

RPC stubs. Remote clients may be recycled and replaced with new instances where the network identities (DNS name to IP address mapping) have changed--. ~~HBASE-14544~~ attempts to work around DNS instability in data centers of years past in a way that, in my opinion, is the wrong thing to do in the modern era. In modern datacenters, in public cloud, and especially in kubernetes environments, DNS mappings are dynamic and subject to frequent change. It is just never the right thing to do to cache them. I intend to propose a revert of ~~HBASE-14544~~. Reverting this simplifies some code a bit. That is the only reason: this is in my opinion some legacy that can be dropped, one fewer configuration variable (yay!), but if this part of the proposal is controversial it can be skipped.
RPC stubs again. When looking up the IP address of the remote host when creating a stub key we also make a key even if the resolution fails. This is the wrong thing to do. If we can't resolve the remote address, we can't contact the server. Making a stub that can't communicate is pointless. Throw an exception instead.
Favored nodes. Although the HDFS API requires InetSocketAddress, we don't have to make up a list right away and cache them forever. We can use Address to record the list of favored nodes and convert from Address to InetSocketAddress on demand (when we go to create the HFile). This will allow us to resolve datanode hostnames just before they are needed. In public cloud, kubernetes, and or some private datacenter service deployment options, datanode servers may have their network identities (DNS name -> IP address mapping) changed over time. We can and should avoid inappropriate caching that may cause us to indefinitely use an incorrect address when contacting a favored node.
Sometimes we use ISA when Address is just as good. For example, the dead servers list. If we are going to pay some attention to ISA usage discipline, let's remove the cases where we use ISA as a host and port pair but do not need to do so. Address works just as well and doesn't present an opportunity for misuse. Another example would be the RPC client concurrentCounterCache.
- We could do a lot more substitutions than what is proposed. All of the ISA uses with bind() are okay as is but could also be updated.

Incorrect cases that cannot be fixed:

hbase-external-blockcache: We have to resolve all of the memcached locations up front because the memcached client constructor requires ISA instances. So we have to hope that the network identities (DNS name -> IP address mapping) does not change for any in the list. This is beyond our control.

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.