zentol commented on a change in pull request #13356:
URL: https://github.com/apache/flink/pull/13356#discussion_r489252488
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/TaskExecutorRegistration.java
##########
@@ -72,13 +77,15 @@ public TaskExecutorRegistration(
final String taskExecutorAddress,
final ResourceID resourceId,
final int dataPort,
+ final int jmxPort,
final HardwareDescription hardwareDescription,
final TaskExecutorMemoryConfiguration
memoryConfiguration,
final ResourceProfile defaultSlotResourceProfile,
final ResourceProfile totalResourceProfile) {
this.taskExecutorAddress = checkNotNull(taskExecutorAddress);
this.resourceId = checkNotNull(resourceId);
this.dataPort = dataPort;
+ this.jmxPort = jmxPort;
Review comment:
I would prefer a more general approach; basically just a Map<String,
String> containing various meta-data bits. Then we could easily add information
about other services and/or ports in the future.
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/taskexecutor/TaskExecutor.java
##########
@@ -1130,6 +1131,7 @@ private void connectToResourceManager() {
getAddress(),
getResourceID(),
unresolvedTaskManagerLocation.getDataPort(),
+ JMXService.getPort().orElse(0),
Review comment:
This strikes me as an odd default value, and I could imagine this being
misunderstood as "running on some random port". `-1` may be a better choice, or
even better, with my above suggestion of using a map, just not including it in
the map at all.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
