[GitHub] [flink] dmvk commented on a diff in pull request #19523: [FLINK-27287][tests] Use random ports

[GitBox <gi...@apache.org>]

dmvk commented on code in PR #19523:
URL: https://github.com/apache/flink/pull/19523#discussion_r854022591


##########
flink-runtime/src/main/java/org/apache/flink/runtime/minicluster/MiniClusterConfiguration.java:
##########
@@ -214,6 +220,15 @@ public MiniClusterConfiguration build() {
                     RestOptions.ADDRESS,
                     modifiedConfiguration.getString(RestOptions.ADDRESS, "localhost"));
 
+            if (useRandomPorts) {
+                if (!configuration.contains(JobManagerOptions.PORT)) {

Review Comment:
   This feels weird. Why not force the options when `useRandomPorts` is enabled?



##########
flink-connectors/flink-connector-kinesis/src/test/java/org/apache/flink/streaming/connectors/kinesis/util/JobManagerWatermarkTrackerTest.java:
##########
@@ -18,57 +18,30 @@
 package org.apache.flink.streaming.connectors.kinesis.util;
 
 import org.apache.flink.configuration.Configuration;
-import org.apache.flink.configuration.RestOptions;
-import org.apache.flink.runtime.minicluster.MiniCluster;
-import org.apache.flink.runtime.minicluster.MiniClusterConfiguration;
+import org.apache.flink.runtime.testutils.MiniClusterResource;
+import org.apache.flink.runtime.testutils.MiniClusterResourceConfiguration;
 import org.apache.flink.streaming.api.environment.StreamExecutionEnvironment;
 import org.apache.flink.streaming.api.functions.sink.SinkFunction;
 import org.apache.flink.streaming.api.functions.source.RichSourceFunction;
 
-import org.junit.AfterClass;
 import org.junit.Assert;
-import org.junit.BeforeClass;
+import org.junit.ClassRule;
 import org.junit.Test;
 
 /** Test for {@link JobManagerWatermarkTracker}. */
 public class JobManagerWatermarkTrackerTest {
 
-    private static MiniCluster flink;
-
-    @BeforeClass
-    public static void setUp() throws Exception {
-        final Configuration config = new Configuration();
-        config.setInteger(RestOptions.PORT, 0);
-
-        final MiniClusterConfiguration miniClusterConfiguration =
-                new MiniClusterConfiguration.Builder()
-                        .setConfiguration(config)
-                        .setNumTaskManagers(1)
-                        .setNumSlotsPerTaskManager(1)
-                        .build();
-
-        flink = new MiniCluster(miniClusterConfiguration);
-
-        flink.start();
-    }
-
-    @AfterClass
-    public static void tearDown() throws Exception {
-        if (flink != null) {
-            flink.close();
-        }
-    }
+    @ClassRule
+    public static final MiniClusterResource FLINK =
+            new MiniClusterResource(
+                    new MiniClusterResourceConfiguration.Builder()

Review Comment:
   `withRandomPorts`?



##########
flink-runtime/src/main/java/org/apache/flink/runtime/minicluster/MiniClusterConfiguration.java:
##########
@@ -206,6 +207,11 @@ public Builder setHaServices(MiniCluster.HaServices haServices) {
             return this;
         }
 
+        public Builder withRandomPorts() {

Review Comment:
   Would it make sense to use random ports by default and have a builder method for the static port allocation? That's IMO what we want in most scenarios.
   
   eg.
   ```
   withStaticPorts(...)
   ```



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org