Poorvankbhatia commented on code in PR #26811:
URL: https://github.com/apache/flink/pull/26811#discussion_r2217265487
##########
flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobServer.java:
##########
@@ -1009,6 +1009,11 @@ public int getPort() {
return this.serverSocket.getLocalPort();
}
+ @Override
+ public InetAddress getAddress() {
+ return this.serverSocket.getInetAddress();
Review Comment:
If BlobServer binds to `0.0.0.0` (wildcard address),
`serverSocket.getInetAddress()` returns `0.0.0.0`. However, clients cannot
connect to `0.0.0.0` as it's not a routable address. Should we consider adding
a check?
##########
flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobService.java:
##########
@@ -43,4 +44,6 @@ public interface BlobService extends Closeable {
* @return the port of the blob server.
*/
int getPort();
+
+ InetAddress getAddress();
Review Comment:
Adding a new method to the `BlobService` interface is a breaking change for
users who have custom implementations of this interface.Should we add or
provide a default implementation?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/blob/NoOpTaskExecutorBlobService.java:
##########
@@ -37,6 +38,11 @@ public int getPort() {
return 0;
}
+ @Override
+ public InetAddress getAddress() {
+ return null;
Review Comment:
Returning `null` here could cause NullPointerException in future tests.
Any test code that calls `blobService.getAddress().getHostName()` will
throw NPE.
Since this is a test utility class, it should return a safe dummy value:
Maybe`return InetAddress.getLoopbackAddress();` ? wdyt.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
