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.
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
[FLINK-12571][network] Make NetworkEnvironment#start() return the binded data port #8496
Conversation
@flinkbot attention @azagrebin |
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
69e53c4
to
70a52e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @zhijiangW ! LGTM 👍
I think we just need a doc comment about the returned port.
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/NetworkEnvironment.java
Show resolved
Hide resolved
Thanks for reviews @azagrebin ! I pushed the fixup commit for adding doc comments and also another hotfix commit for removing legacy |
flink-runtime/src/main/java/org/apache/flink/runtime/io/network/ConnectionManager.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :) I will merge with green travis. Can you ping me once it's green?
Thanks for reviews @pnowojski . It is already green. |
…ded data port NetworkEnvironment#getConnectionManager is currently used for getting binded data port from ConnectionManager. Considering the general shuffle service architecture, the internal ConnectionManager in NetworkEnvironment should not be exposed to outsides. We could make ShuffleService#start return the binded data port directly if exists, then for other cases it could return a default int value which seems no harm.
Merging. |
What is the purpose of the change
NetworkEnvironment#getConnectionManager
is currently used for getting binded data port fromConnectionManager
. Considering the general shuffle service architecture, the internalConnectionManager
inNetworkEnvironment
should not be exposed to outsides. We could makeShuffleService#start
return the binded data port directly if exists, then for other cases it could return a default int value which seems no harm.Brief change log
ConnectionManager#start
return binded data port.NetworkEnvironment#start
return binded data portNettyServer
TaskManagerServices
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation