[GitHub] [flink] rmetzger commented on a change in pull request #16265: [FLINK-23090] Introduce RpcSystem abstraction

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

rmetzger commented on a change in pull request #16265:
URL: https://github.com/apache/flink/pull/16265#discussion_r658142891



##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/MetricRegistryConfiguration.java
##########
@@ -70,13 +70,19 @@ public long getQueryServiceMessageSizeLimit() {
     //  Static factory methods
     // ------------------------------------------------------------------------
 
+    @VisibleForTesting
+    public static MetricRegistryConfiguration fromConfiguration(Configuration configuration) {
+        return fromConfiguration(configuration, 10485760);
+    }

Review comment:
       I'm wondering if we can make this nicer? Introduce a constant, or remove this static method and pass the max frame size from the tests.
   I'm concerned about having a public method that is not strictly needed "polluting" the interface.

##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/rpc/RpcUtils.java
##########
@@ -158,6 +158,10 @@ public static String getHostname(RpcService rpcService) {
                 : rpcServiceAddress;
     }
 
+    public static RpcSystem.ForkJoinExecutorConfiguration getTestForkJoinExecutorConfiguration() {
+        return new RpcSystem.ForkJoinExecutorConfiguration(1.0, 2, 4);
+    }
+

Review comment:
       I wonder if this would be better placed in the MiniCluster class, where it is only used?
   This seems a bit random here.




-- 
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:
users@infra.apache.org