Skip to content

[FLINK-38109] [flink-runtime] Fix BlobClient address when bindhost is set #26811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 9, 2025
Merged

[FLINK-38109] [flink-runtime] Fix BlobClient address when bindhost is set #26811

merged 1 commit into from
Sep 9, 2025
+137 −17

Conversation

rosakng
Copy link
Contributor

@rosakng rosakng commented Jul 18, 2025 •
edited
Loading

https://issues.apache.org/jira/browse/FLINK-38109

What is the purpose of the change

The current implementation of BlobServer is to inherit the JobManager bind-host, however theres implicit assumptions that the BlobClient can use the dispatcher gateway host name.
This pull request surfaces the BlobServer hostname directly to the BlobClient, similar to how the BlobServer port works today.

Brief change log

  • BlobClient uses the correct host name when the JobManager bind-host is set.

Verifying this change

Updates the current BlobClientTest to use the BlobServer address instead of localhost.
Additional tests added for blobserver binding to a routable address and a configured address inherited by the JobManager bind-host.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
  • The serializers: (yes / no / don't know)
  • The runtime per-record code paths (performance sensitive): (yes / no / don't know)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
  • The S3 file system connector: (yes / no / don't know)

no to all

Documentation

  • Does this pull request introduce a new feature? (yes / no)
    no
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
    not applicable

@flinkbot
Copy link
Collaborator

flinkbot commented Jul 18, 2025 •
edited
Loading

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

Poorvankbhatia
Poorvankbhatia reviewed Jul 19, 2025
View reviewed changes
flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobServer.java Outdated
@@ -1009,6 +1009,11 @@ public int getPort() {
return this.serverSocket.getLocalPort();
}

@Override
public InetAddress getAddress() {
return this.serverSocket.getInetAddress();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a legitimate issue. Clients cannot use 0.0.0.0 for connections. Adding a check here to fallback to the local address sounds reasonable to me

Poorvankbhatia
Poorvankbhatia reviewed Jul 19, 2025
View reviewed changes
flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobService.java Outdated
@@ -43,4 +44,6 @@ public interface BlobService extends Closeable {
* @return the port of the blob server.
*/
int getPort();

InetAddress getAddress();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@rosakng rosakng Jul 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me. Added a default implementation and removed the unnecessary override so that it falls through to using the LoopbackAddress 👍

Poorvankbhatia
Poorvankbhatia reviewed Jul 19, 2025
View reviewed changes
Copy link
Contributor

@Poorvankbhatia Poorvankbhatia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the existing tests have been updated to use getBlobServer().getAddress(), there's no test that specifically reproduces the bug described in FLINK-38109.
Can u consider adding a test that verifies the fix works when BlobServer binds to a different address?

@github-actions github-actions bot added community-reviewed PR has been reviewed by the community. and removed community-reviewed PR has been reviewed by the community. labels Jul 19, 2025
@rosakng rosakng force-pushed the FLINK-38109 branch 3 times, most recently from 30c527f to 93782b0 Compare July 23, 2025 20:27
@rosakng
Copy link
Contributor Author

rosakng commented Jul 23, 2025

While the existing tests have been updated to use getBlobServer().getAddress(), there's no test that specifically reproduces the bug described in FLINK-38109. Can u consider adding a test that verifies the fix works when BlobServer binds to a different address?

@Poorvankbhatia Thanks for the suggestion. I've added additional tests for the blobserver binding to a routable address and a configured address inherited by the JobManager bind-host.

@github-actions github-actions bot added community-reviewed PR has been reviewed by the community. and removed community-reviewed PR has been reviewed by the community. labels Jul 24, 2025
Poorvankbhatia
Poorvankbhatia approved these changes Jul 24, 2025
View reviewed changes
Copy link
Contributor

@Poorvankbhatia Poorvankbhatia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@github-actions github-actions bot added community-reviewed PR has been reviewed by the community. and removed community-reviewed PR has been reviewed by the community. labels Jul 24, 2025
@github-actions github-actions bot removed the community-reviewed PR has been reviewed by the community. label Jul 27, 2025
@github-actions github-actions bot added community-reviewed PR has been reviewed by the community. and removed community-reviewed PR has been reviewed by the community. labels Aug 26, 2025
@github-actions github-actions bot added community-reviewed PR has been reviewed by the community. and removed community-reviewed PR has been reviewed by the community. labels Sep 3, 2025
ferenc-csaky
ferenc-csaky approved these changes Sep 9, 2025
View reviewed changes
Copy link
Contributor

@ferenc-csaky ferenc-csaky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution @rosakng, and the thorough review @Poorvankbhatia! LGTM

@ferenc-csaky ferenc-csaky merged commit deee02b into apache:master Sep 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ferenc-csaky ferenc-csaky ferenc-csaky approved these changes

+1 more reviewer

@Poorvankbhatia Poorvankbhatia Poorvankbhatia approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
community-reviewed PR has been reviewed by the community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rosakng @flinkbot @Poorvankbhatia @ferenc-csaky