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<String> portKeys =
LaunchableMesosWorker.extractPortKeys(config);
+
+ // Assert
+ assertEquals("Must get right number of port keys", 2,
portKeys.size());
+ Iterator<String> 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
With regards,
Apache Git Services
