You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "nodece (via GitHub)" <gi...@apache.org> on 2025/04/16 14:17:47 UTC
[PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
nodece opened a new pull request, #4588:
URL: https://github.com/apache/bookkeeper/pull/4588
### Motivation
The existing implementation resolves the default hostname for the network interface early, even when it's not required. This can lead to unnecessary DNS resolution or even failures in environments where hostname mappings are missing or incomplete.
The goal is to simplify and make host address resolution more robust by:
- Using the default IP address as the starting point.
- Performing reverse DNS lookup (IP → hostname) only when `useHostNameAsBookieID` is enabled.
- Avoiding unnecessary dependency on hostname resolution when IP-based Bookie IDs are sufficient.
### Changes
- Replaced `DNS.getDefaultHost(iface)` with `DNS.getDefaultIP(iface)` as the default starting point.
- Introduced conditional logic to resolve the hostname from the IP only when `useHostNameAsBookieID` is true.
- Simplified code structure and reduced redundant resolution steps.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "liudezhi2098 (via GitHub)" <gi...@apache.org>.
liudezhi2098 commented on PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#issuecomment-2817497774
Good jobs
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#discussion_r2048333396
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java:
##########
@@ -267,15 +267,14 @@ public static BookieSocketAddress getBookieAddress(ServerConfiguration conf)
iface = "default";
}
- String hostName = DNS.getDefaultHost(iface);
- InetSocketAddress inetAddr = new InetSocketAddress(hostName, conf.getBookiePort());
- if (inetAddr.isUnresolved()) {
- throw new UnknownHostException("Unable to resolve default hostname: "
- + hostName + " for interface: " + iface);
- }
- String hostAddress = null;
- InetAddress iAddress = inetAddr.getAddress();
+ String hostAddress = DNS.getDefaultIP(iface);
if (conf.getUseHostNameAsBookieID()) {
+ InetSocketAddress inetAddr = new InetSocketAddress(hostAddress, conf.getBookiePort());
+ if (inetAddr.isUnresolved()) {
+ throw new UnknownHostException("Unable to resolve default hostaddress: "
+ + hostAddress + " for interface: " + iface);
+ }
+ InetAddress iAddress = inetAddr.getAddress();
Review Comment:
This original code itself is not great. This could be improved further since there is no need to use `InetSocketAddress` here. This could be replaced by the usage of `java.net.InetAddress#getByName(java.lang.String)`
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#discussion_r2048488033
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java:
##########
@@ -267,25 +266,16 @@ public static BookieSocketAddress getBookieAddress(ServerConfiguration conf)
iface = "default";
}
- String hostName = DNS.getDefaultHost(iface);
- InetSocketAddress inetAddr = new InetSocketAddress(hostName, conf.getBookiePort());
- if (inetAddr.isUnresolved()) {
- throw new UnknownHostException("Unable to resolve default hostname: "
- + hostName + " for interface: " + iface);
- }
- String hostAddress = null;
- InetAddress iAddress = inetAddr.getAddress();
+ String hostAddress = DNS.getDefaultIP(iface);
if (conf.getUseHostNameAsBookieID()) {
- hostAddress = iAddress.getCanonicalHostName();
+ hostAddress = InetAddress.getByName(hostAddress).getCanonicalHostName();
Review Comment:
I guess the main value is that it adds the interface information to the exception message
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#issuecomment-2820056497
/cc @lhotari
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#discussion_r2048570802
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java:
##########
@@ -267,25 +266,16 @@ public static BookieSocketAddress getBookieAddress(ServerConfiguration conf)
iface = "default";
}
- String hostName = DNS.getDefaultHost(iface);
- InetSocketAddress inetAddr = new InetSocketAddress(hostName, conf.getBookiePort());
- if (inetAddr.isUnresolved()) {
- throw new UnknownHostException("Unable to resolve default hostname: "
- + hostName + " for interface: " + iface);
- }
- String hostAddress = null;
- InetAddress iAddress = inetAddr.getAddress();
+ String hostAddress = DNS.getDefaultIP(iface);
if (conf.getUseHostNameAsBookieID()) {
- hostAddress = iAddress.getCanonicalHostName();
+ hostAddress = InetAddress.getByName(hostAddress).getCanonicalHostName();
Review Comment:
Makes sense.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#issuecomment-2809745878
This PR builds on and includes the changes from [#4583](https://github.com/apache/bookkeeper/pull/4583), further simplifying the resolution logic and improving robustness.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#discussion_r2048414928
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java:
##########
@@ -267,15 +267,14 @@ public static BookieSocketAddress getBookieAddress(ServerConfiguration conf)
iface = "default";
}
- String hostName = DNS.getDefaultHost(iface);
- InetSocketAddress inetAddr = new InetSocketAddress(hostName, conf.getBookiePort());
- if (inetAddr.isUnresolved()) {
- throw new UnknownHostException("Unable to resolve default hostname: "
- + hostName + " for interface: " + iface);
- }
- String hostAddress = null;
- InetAddress iAddress = inetAddr.getAddress();
+ String hostAddress = DNS.getDefaultIP(iface);
if (conf.getUseHostNameAsBookieID()) {
+ InetSocketAddress inetAddr = new InetSocketAddress(hostAddress, conf.getBookiePort());
+ if (inetAddr.isUnresolved()) {
+ throw new UnknownHostException("Unable to resolve default hostaddress: "
+ + hostAddress + " for interface: " + iface);
+ }
+ InetAddress iAddress = inetAddr.getAddress();
Review Comment:
@lhotari Done.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#issuecomment-2814484727
There is nothing wrong with the code, but it is recommended to add a testcase to cover this fix
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#issuecomment-2820056238
> There is nothing wrong with the code, but it is recommended to add a testcase to cover this fix @nodece
@StevenLuMT Although the existing tests can cover these changes, if the `java.net.InetAddress#getCanonicalHostName` cannot resolve the IP address to the hostname, which returns the IP address, we should avoid this case.
> Gets the fully qualified domain name for this IP address. Best effort method, meaning we may not be able to return the FQDN depending on the underlying system configuration.
If there is a security manager, this method first calls its checkConnect method with the hostname and -1 as its arguments to see if the calling code is allowed to know the hostname for this IP address, i.e., to connect to the host. If the operation is not allowed, it will return the textual representation of the IP address.
Returns:
the fully qualified domain name for this IP address, or if the operation is not allowed by the security check, the textual representation of the IP address.
So I submit https://github.com/apache/bookkeeper/pull/4595 to fix this, could you have a chance to review?
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#discussion_r2048486974
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java:
##########
@@ -267,25 +266,16 @@ public static BookieSocketAddress getBookieAddress(ServerConfiguration conf)
iface = "default";
}
- String hostName = DNS.getDefaultHost(iface);
- InetSocketAddress inetAddr = new InetSocketAddress(hostName, conf.getBookiePort());
- if (inetAddr.isUnresolved()) {
- throw new UnknownHostException("Unable to resolve default hostname: "
- + hostName + " for interface: " + iface);
- }
- String hostAddress = null;
- InetAddress iAddress = inetAddr.getAddress();
+ String hostAddress = DNS.getDefaultIP(iface);
if (conf.getUseHostNameAsBookieID()) {
- hostAddress = iAddress.getCanonicalHostName();
+ hostAddress = InetAddress.getByName(hostAddress).getCanonicalHostName();
Review Comment:
I wonder if it would be useful to catch `UnknownHostException` and throw a new one in the same way as the original code was doing?
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#issuecomment-2814402215
@StevenLuMT @hezhangjian Could you have a chance to review?
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] improve: Improve host address resolution logic for bookie id [bookkeeper]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on PR #4588:
URL: https://github.com/apache/bookkeeper/pull/4588#issuecomment-2814492996
> There is nothing wrong with the code, but it is recommended to add a testcase to cover this fix @nodece
@StevenLuMT Thanks for the suggestion. The fix is in `BookieImpl.java`, which is a core class that's already covered by multiple existing tests. Since the logic is indirectly exercised through various integration and unit tests, adding a separate test might be meaningless.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org