[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16706765#comment-16706765
 ] 

ASF GitHub Bot commented on FLINK-10149:


asfgit closed pull request #7203: [FLINK-10149[mesos] Don't allocate extra 
mesos port for TM unless configured to do so
URL: https://github.com/apache/flink/pull/7203
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git 
a/flink-mesos/src/main/java/org/apache/flink/mesos/configuration/MesosOptions.java
 
b/flink-mesos/src/main/java/org/apache/flink/mesos/configuration/MesosOptions.java
index 426a891e814..0c4e1f6bcba 100644
--- 
a/flink-mesos/src/main/java/org/apache/flink/mesos/configuration/MesosOptions.java
+++ 
b/flink-mesos/src/main/java/org/apache/flink/mesos/configuration/MesosOptions.java
@@ -136,8 +136,9 @@
/**
 * Config parameter to configure which configuration keys will 
dynamically get a port assigned through Mesos.
 */
-   public static final ConfigOption PORT_ASSIGNMENTS = 
key("mesos.resourcemanager.tasks.port-assignments")
-   .defaultValue("")
+   public static final ConfigOption PORT_ASSIGNMENTS =
+   key("mesos.resourcemanager.tasks.port-assignments")
+   .noDefaultValue()
.withDescription(Description.builder()
.text("Comma-separated list of configuration keys which 
represent a configurable port. " +
"All port keys will dynamically get a port 
assigned through Mesos.")
diff --git 
a/flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
 
b/flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
index 84ec2229a2a..637442c899d 100644
--- 
a/flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
+++ 
b/flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
@@ -41,6 +41,7 @@
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -64,12 +65,13 @@
 public class LaunchableMesosWorker implements LaunchableTask {
 
protected static final Logger LOG = 
LoggerFactory.getLogger(LaunchableMesosWorker.class);
+
/**
 * The set of configuration keys to be dynamically configured with a 
port allocated from Mesos.
 */
-   static final String[] TM_PORT_KEYS = {
+   static final Set TM_PORT_KEYS = Collections.unmodifiableSet(new 
HashSet<>(Arrays.asList(
"taskmanager.rpc.port",
-   "taskmanager.data.port"};
+   "taskmanager.data.port")));
 
private final MesosArtifactResolver resolver;
private final ContainerSpecification containerSpec;
@@ -342,16 +344,18 @@ public String toString() {
 * @return A deterministically ordered Set of port keys to expose from 
the TM container
 */
static Set extractPortKeys(Configuration config) {
-   final LinkedHashSet tmPortKeys = new 
LinkedHashSet<>(Arrays.asList(TM_PORT_KEYS));
+   final LinkedHashSet tmPortKeys = new 
LinkedHashSet<>(TM_PORT_KEYS);
 
final String portKeys = config.getString(PORT_ASSIGNMENTS);
 
-   Arrays.stream(portKeys.split(","))
-   .map(String::trim)
-   .peek(key -> LOG.debug("Adding port key " + key + " to 
mesos request"))
-   .forEach(tmPortKeys::add);
+   if (portKeys != null) {
+   Arrays.stream(portKeys.split(","))
+   .map(String::trim)
+   .peek(key -> LOG.debug("Adding port key {} to 
mesos request"))
+   .forEach(tmPortKeys::add);
+   }
 
-   return tmPortKeys;
+   return Collections.unmodifiableSet(tmPortKeys);
}
 
@Override
diff --git 
a/flink-mesos/src/test/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorkerTest.java
 
b/flink-mesos/src/test/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorkerTest.java
index 6784e427c1f..48a436cb995 100644
--- 
a/flink-mesos/src/test/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorkerTest.java
+++ 
b/flink-mesos/src/test/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorkerTest.java
@@ -23,11 +23,14 @@
 
 import org.junit.Test;
 
-import java.util.Iterator;
+import java.util.Arrays;
+import 

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16704573#comment-16704573
 ] 

ASF GitHub Bot commented on FLINK-10149:


GJL commented on issue #6557: [FLINK-10149] [flink-mesos] Don't allocate extra 
mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557#issuecomment-443168598
 
 
   Closing because subsumed by #7203


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>  Components: Mesos
>Affects Versions: 1.6.1, 1.7.0
>Reporter: Rune Skou Larsen
>Assignee: Gary Yao
>Priority: Minor
>  Labels: pull-request-available
> Fix For: 1.6.3, 1.8.0, 1.7.1
>
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16704574#comment-16704574
 ] 

ASF GitHub Bot commented on FLINK-10149:


GJL closed pull request #6557: [FLINK-10149] [flink-mesos] Don't allocate extra 
mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git 
a/flink-mesos/src/main/java/org/apache/flink/mesos/configuration/MesosOptions.java
 
b/flink-mesos/src/main/java/org/apache/flink/mesos/configuration/MesosOptions.java
index 426a891e814..0c4e1f6bcba 100644
--- 
a/flink-mesos/src/main/java/org/apache/flink/mesos/configuration/MesosOptions.java
+++ 
b/flink-mesos/src/main/java/org/apache/flink/mesos/configuration/MesosOptions.java
@@ -136,8 +136,9 @@
/**
 * Config parameter to configure which configuration keys will 
dynamically get a port assigned through Mesos.
 */
-   public static final ConfigOption PORT_ASSIGNMENTS = 
key("mesos.resourcemanager.tasks.port-assignments")
-   .defaultValue("")
+   public static final ConfigOption PORT_ASSIGNMENTS =
+   key("mesos.resourcemanager.tasks.port-assignments")
+   .noDefaultValue()
.withDescription(Description.builder()
.text("Comma-separated list of configuration keys which 
represent a configurable port. " +
"All port keys will dynamically get a port 
assigned through Mesos.")
diff --git 
a/flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
 
b/flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
index bb15aee0a7f..e906ade3a72 100644
--- 
a/flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
+++ 
b/flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
@@ -346,10 +346,12 @@ public String toString() {
 
final String portKeys = config.getString(PORT_ASSIGNMENTS);
 
-   Arrays.stream(portKeys.split(","))
-   .map(String::trim)
-   .peek(key -> LOG.debug("Adding port key " + key + " to 
mesos request"))
-   .forEach(tmPortKeys::add);
+   if (portKeys != null) {
+   Arrays.stream(portKeys.split(","))
+   .map(String::trim)
+   .peek(key -> LOG.debug("Adding port key " + key 
+ " to mesos request"))
+   .forEach(tmPortKeys::add);
+   }
 
return tmPortKeys;
}
diff --git 
a/flink-mesos/src/test/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorkerTest.java
 
b/flink-mesos/src/test/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorkerTest.java
index 6784e427c1f..7fc99d28db3 100644
--- 
a/flink-mesos/src/test/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorkerTest.java
+++ 
b/flink-mesos/src/test/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorkerTest.java
@@ -52,4 +52,19 @@ public void canGetPortKeys() {
assertEquals("port key must be correct", "anotherport", 
iterator.next());
}
 
+   @Test
+   public void canGetNoPortKeys() {
+   // Setup
+   Configuration config = new Configuration();
+
+   // Act
+   Set portKeys = 
LaunchableMesosWorker.extractPortKeys(config);
+
+   // Assert
+   assertEquals("Must get right number of port keys", 2, 
portKeys.size());
+   Iterator iterator = portKeys.iterator();
+   assertEquals("port key must be correct", 
LaunchableMesosWorker.TM_PORT_KEYS[0], iterator.next());
+   assertEquals("port key must be correct", 
LaunchableMesosWorker.TM_PORT_KEYS[1], iterator.next());
+   }
+
 }


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>  

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16704570#comment-16704570
 ] 

ASF GitHub Bot commented on FLINK-10149:


GJL opened a new pull request #7203: [FLINK-10149[mesos] Don't allocate extra 
mesos port for TM unless configured to do so
URL: https://github.com/apache/flink/pull/7203
 
 
   ## What is the purpose of the change
   
   *Make sure Flink only allocates the necessary mesos ports. This PR is based 
on #6557*
   
   cc: @runesl 
   
   ## Brief change log
   
 - *Remove default value for config option, and treat absence correctly.*
 - *See commits*
   
   ## Verifying this change
   
   This change added tests and can be verified as follows:
   
 - *Added unit test in `LaunchableMesosWorkerTest`*
   
   ## 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, Yarn/Mesos, ZooKeeper: (**yes** / no / don't know)
 - The S3 file system connector: (yes / **no** / don't know)
   
   ## Documentation
   
 - Does this pull request introduce a new feature? (yes / **no**)
 - If yes, how is the feature documented? (**not applicable** / docs / 
JavaDocs / not documented)
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>  Components: Mesos
>Affects Versions: 1.6.1, 1.7.0
>Reporter: Rune Skou Larsen
>Assignee: Gary Yao
>Priority: Minor
>  Labels: pull-request-available
> Fix For: 1.6.3, 1.8.0, 1.7.1
>
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16655346#comment-16655346
 ] 

ASF GitHub Bot commented on FLINK-10149:


GJL removed a comment on issue #6557: [FLINK-10149] [flink-mesos] Don't 
allocate extra mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557#issuecomment-431026453
 
 
   I have a question about code that was not touched in this PR but in an 
earlier one. Below is an extract from `LaunchableMesosWorker#launch()`:
   ```
   Set tmPortKeys = 
extractPortKeys(containerSpec.getDynamicConfiguration());
   List portResources = allocation.takeRanges("ports", 
tmPortKeys.size(), roles);
   taskInfo.addAllResources(portResources);
   Iterator portsToAssign = tmPortKeys.iterator();
   rangeValues(portResources).forEach(port -> 
dynamicProperties.setLong(portsToAssign.next(), port));
   if (portsToAssign.hasNext()) {
throw new IllegalArgumentException("insufficient # of ports assigned");
   }
   ```
   Judging by the code, I can only assume that port ranges are not allowed, 
i.e., it is illegal to receive a port `Resource` where `begin != end`. I am not 
a Mesos expert – can you tell me whether my assumption is correct? If yes, 
additional checks should be added to `rangeValues` because this function allows 
port ranges, which can cause confusion.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>Reporter: Rune Skou Larsen
>Assignee: Rune Skou Larsen
>Priority: Minor
>  Labels: pull-request-available
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16655344#comment-16655344
 ] 

ASF GitHub Bot commented on FLINK-10149:


GJL commented on a change in pull request #6557: [FLINK-10149] [flink-mesos] 
Don't allocate extra mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557#discussion_r226269544
 
 

 ##
 File path: 
flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
 ##
 @@ -346,10 +346,12 @@ public String toString() {
 
final String portKeys = config.getString(PORT_ASSIGNMENTS);
 
-   Arrays.stream(portKeys.split(","))
-   .map(String::trim)
-   .peek(key -> LOG.debug("Adding port key " + key + " to 
mesos request"))
-   .forEach(tmPortKeys::add);
+   if (portKeys != null) {
+   Arrays.stream(portKeys.split(","))
+   .map(String::trim)
+   .peek(key -> LOG.debug("Adding port key " + key 
+ " to mesos request"))
 
 Review comment:
   Slf4j placeholders should be preferred, i.e., `LOG.debug("Adding port key {} 
to mesos request", key)`


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>Reporter: Rune Skou Larsen
>Assignee: Rune Skou Larsen
>Priority: Minor
>  Labels: pull-request-available
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16655345#comment-16655345
 ] 

ASF GitHub Bot commented on FLINK-10149:


GJL commented on a change in pull request #6557: [FLINK-10149] [flink-mesos] 
Don't allocate extra mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557#discussion_r226332854
 
 

 ##
 File path: 
flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
 ##
 @@ -346,10 +346,12 @@ public String toString() {
 
final String portKeys = config.getString(PORT_ASSIGNMENTS);
 
-   Arrays.stream(portKeys.split(","))
-   .map(String::trim)
-   .peek(key -> LOG.debug("Adding port key " + key + " to 
mesos request"))
-   .forEach(tmPortKeys::add);
+   if (portKeys != null) {
+   Arrays.stream(portKeys.split(","))
+   .map(String::trim)
+   .peek(key -> LOG.debug("Adding port key " + key 
+ " to mesos request"))
+   .forEach(tmPortKeys::add);
+   }
 
return tmPortKeys;
 
 Review comment:
   I'd wrap the return value in `Collections.unmodifiableSet`.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>Reporter: Rune Skou Larsen
>Assignee: Rune Skou Larsen
>Priority: Minor
>  Labels: pull-request-available
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16655343#comment-16655343
 ] 

ASF GitHub Bot commented on FLINK-10149:


GJL commented on a change in pull request #6557: [FLINK-10149] [flink-mesos] 
Don't allocate extra mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557#discussion_r226332516
 
 

 ##
 File path: 
flink-mesos/src/test/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorkerTest.java
 ##
 @@ -52,4 +52,19 @@ public void canGetPortKeys() {
assertEquals("port key must be correct", "anotherport", 
iterator.next());
}
 
+   @Test
+   public void canGetNoPortKeys() {
+   // Setup
+   Configuration config = new Configuration();
+
+   // Act
+   Set portKeys = 
LaunchableMesosWorker.extractPortKeys(config);
+
+   // Assert
+   assertEquals("Must get right number of port keys", 2, 
portKeys.size());
+   Iterator iterator = portKeys.iterator();
+   assertEquals("port key must be correct", 
LaunchableMesosWorker.TM_PORT_KEYS[0], iterator.next());
 
 Review comment:
   I find this test, and the implementation of `extractPortKeys` not ideal.
   
   1. From the signature of `extractPortKeys` it is not obvious that the 
elements will be ordered.
   1. Generally speaking there is no order imposed on the elements of a `Set` 
but the order of iteration should be deterministic for all collections [1], 
i.e., if you iterate twice over the same instance of the collection, you get 
the same order, which should be good enough for your use case.
   1. Considering all above, the assertions can be simplified as 
   ```
   assertThat(portKeys, 
containsInAnyOrder(LaunchableMesosWorker.TM_PORT_KEYS[0], 
LaunchableMesosWorker.TM_PORT_KEYS[1]));
   ```
   Note that this also asserts that the size matches.
   
   [1] https://stackoverflow.com/a/27210701/9203198


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>Reporter: Rune Skou Larsen
>Assignee: Rune Skou Larsen
>Priority: Minor
>  Labels: pull-request-available
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16655285#comment-16655285
 ] 

ASF GitHub Bot commented on FLINK-10149:


GJL commented on issue #6557: [FLINK-10149] [flink-mesos] Don't allocate extra 
mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557#issuecomment-431026453
 
 
   I have a question about code that was not touched in this PR but in an 
earlier one. Below is an extract from `LaunchableMesosWorker#launch()`:
   ```
   Set tmPortKeys = 
extractPortKeys(containerSpec.getDynamicConfiguration());
   List portResources = allocation.takeRanges("ports", 
tmPortKeys.size(), roles);
   taskInfo.addAllResources(portResources);
   Iterator portsToAssign = tmPortKeys.iterator();
   rangeValues(portResources).forEach(port -> 
dynamicProperties.setLong(portsToAssign.next(), port));
   if (portsToAssign.hasNext()) {
throw new IllegalArgumentException("insufficient # of ports assigned");
   }
   ```
   Judging by the code, I can only assume that port ranges are not allowed, 
i.e., it is illegal to receive a port `Resource` where `begin != end`. I am not 
a Mesos expert – can you tell me whether my assumption is correct? If yes, 
additional checks should be added to `rangeValues` because this function allows 
port ranges, which can cause confusion.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>Reporter: Rune Skou Larsen
>Assignee: Rune Skou Larsen
>Priority: Minor
>  Labels: pull-request-available
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16655078#comment-16655078
 ] 

ASF GitHub Bot commented on FLINK-10149:


GJL commented on a change in pull request #6557: [FLINK-10149] [flink-mesos] 
Don't allocate extra mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557#discussion_r226269500
 
 

 ##
 File path: 
flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
 ##
 @@ -346,10 +346,12 @@ public String toString() {
 
final String portKeys = config.getString(PORT_ASSIGNMENTS);
 
-   Arrays.stream(portKeys.split(","))
-   .map(String::trim)
-   .peek(key -> LOG.debug("Adding port key " + key + " to 
mesos request"))
-   .forEach(tmPortKeys::add);
+   if (portKeys != null) {
+   Arrays.stream(portKeys.split(","))
+   .map(String::trim)
+   .peek(key -> LOG.debug("Adding port key " + key 
+ " to mesos request"))
 
 Review comment:
   Slf4j placeholders should be preferred, i.e., `LOG.debug("Adding port key {} 
to mesos request", key)`


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>Reporter: Rune Skou Larsen
>Assignee: Rune Skou Larsen
>Priority: Minor
>  Labels: pull-request-available
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16655077#comment-16655077
 ] 

ASF GitHub Bot commented on FLINK-10149:


GJL commented on a change in pull request #6557: [FLINK-10149] [flink-mesos] 
Don't allocate extra mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557#discussion_r226269500
 
 

 ##
 File path: 
flink-mesos/src/main/java/org/apache/flink/mesos/runtime/clusterframework/LaunchableMesosWorker.java
 ##
 @@ -346,10 +346,12 @@ public String toString() {
 
final String portKeys = config.getString(PORT_ASSIGNMENTS);
 
-   Arrays.stream(portKeys.split(","))
-   .map(String::trim)
-   .peek(key -> LOG.debug("Adding port key " + key + " to 
mesos request"))
-   .forEach(tmPortKeys::add);
+   if (portKeys != null) {
+   Arrays.stream(portKeys.split(","))
+   .map(String::trim)
+   .peek(key -> LOG.debug("Adding port key " + key 
+ " to mesos request"))
 
 Review comment:
   Slf4j placeholders should be preferred, i.e., `LOG.debug("Adding port key {} 
to mesos request", key)`


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>Reporter: Rune Skou Larsen
>Assignee: Rune Skou Larsen
>Priority: Minor
>  Labels: pull-request-available
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16625811#comment-16625811
 ] 

ASF GitHub Bot commented on FLINK-10149:


rsltrifork commented on issue #6557: [FLINK-10149] [flink-mesos] Don't allocate 
extra mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557#issuecomment-423972275
 
 
   @yanghua Thanks for reviewing and sorry for taking so long to corect the 
formatting. I hope everything is ok now.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>Reporter: Rune Skou Larsen
>Assignee: Rune Skou Larsen
>Priority: Minor
>  Labels: pull-request-available
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16582687#comment-16582687
 ] 

ASF GitHub Bot commented on FLINK-10149:


yanghua commented on a change in pull request #6557: [FLINK-10149] 
[flink-mesos] Don't allocate extra mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557#discussion_r210638112
 
 

 ##
 File path: 
flink-mesos/src/main/java/org/apache/flink/mesos/configuration/MesosOptions.java
 ##
 @@ -137,7 +137,7 @@
 * Config parameter to configure which configuration keys will 
dynamically get a port assigned through Mesos.
 */
public static final ConfigOption PORT_ASSIGNMENTS = 
key("mesos.resourcemanager.tasks.port-assignments")
 
 Review comment:
   change : 
   
   ```java
   public static final ConfigOption PORT_ASSIGNMENTS = 
key("mesos.resourcemanager.tasks.port-assignments")
   ```
   
   to : 
   
   ```java
   public static final ConfigOption PORT_ASSIGNMENTS = 
   key("mesos.resourcemanager.tasks.port-assignments")
   ```
   
   let it match other configuration item's style,  looks better to me.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>Reporter: Rune Skou Larsen
>Assignee: Rune Skou Larsen
>Priority: Minor
>  Labels: pull-request-available
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

[jira] [Commented] (FLINK-10149) Fink Mesos allocates extra port when not configured to do so.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-10149?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16580917#comment-16580917
 ] 

ASF GitHub Bot commented on FLINK-10149:


rsltrifork opened a new pull request #6557: [FLINK-10149] [flink-mesos] Don't 
allocate extra mesos port for TM unless configured to do so.
URL: https://github.com/apache/flink/pull/6557
 
 
   ## What is the purpose of the change
   
   Make sure Flink only allocates the necessary mesos ports.
   
   ## Brief change log
   Remove default value for config option, and treat absence correctly.
   
   ## Verifying this change
 - Added unit test
   
   ## Documentation
 - Does this pull request introduce a new feature? **no**
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


> Fink Mesos allocates extra port when not configured to do so.
> -
>
> Key: FLINK-10149
> URL: https://issues.apache.org/jira/browse/FLINK-10149
> Project: Flink
>  Issue Type: Bug
>Reporter: Rune Skou Larsen
>Assignee: Rune Skou Larsen
>Priority: Minor
>  Labels: pull-request-available
>
> Internal testing has revealed a small bug in the way LaunchableMesosWorker 
> handles the absense of the new *mesos.resourcemanager.tasks.port-assignments* 
> config option.
> It allocates an extra mesos port even when the option is not set.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)