xintongsong commented on code in PR #24815:
URL: https://github.com/apache/flink/pull/24815#discussion_r1609367863


##########
flink-runtime/src/main/java/org/apache/flink/runtime/dispatcher/Dispatcher.java:
##########
@@ -140,7 +141,7 @@
 public abstract class Dispatcher extends FencedRpcEndpoint<DispatcherId>
         implements DispatcherGateway {
 
-    @VisibleForTesting
+    @Internal

Review Comment:
   `@VisibleForTesting` should not be removed. This field is `public` only for 
the testing purpose. Without testing, the filed can be `private` because no 
production codes outside `Dispatcher` uses it.



##########
flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/NettyShuffleEnvironmentConfiguration.java:
##########
@@ -378,7 +378,9 @@ public static NettyShuffleEnvironmentConfiguration 
fromConfiguration(
         boolean batchShuffleCompressionEnabled =
                 
configuration.get(NettyShuffleEnvironmentOptions.BATCH_SHUFFLE_COMPRESSION_ENABLED);
         String compressionCodec =
-                
configuration.get(NettyShuffleEnvironmentOptions.SHUFFLE_COMPRESSION_CODEC);
+                configuration
+                        
.get(NettyShuffleEnvironmentOptions.SHUFFLE_COMPRESSION_CODEC)
+                        .toString();

Review Comment:
   Why would we want to convert this enum to a string. Looking into all its 
usages, eventually the string will be passed into 
`BlockCompressionFactory#createBlockCompressionFactory` and convert to 
`CompressionFactoryName`.



##########
flink-core/src/main/java/org/apache/flink/configuration/NettyShuffleEnvironmentOptions.java:
##########
@@ -121,6 +121,12 @@ public class NettyShuffleEnvironmentOptions {
                                     + "speed is the slowest, and LZO is 
between the two. Also note "
                                     + "that this option is experimental and 
might be changed in the future.");
 
+    public enum ShuffleCompressionCodec {
+        LZ4,
+        LZO,
+        ZSTD
+    }

Review Comment:
   How about we just use `BlockCompressionFactory#CompressionFactoryName`, 
moving it to some API module / package and maybe also rename it to 
`ShuffleCompressionCodec`.



##########
flink-core/src/main/java/org/apache/flink/configuration/ConfigurationUtils.java:
##########
@@ -140,12 +140,12 @@ public static String parseMapToString(Map<String, String> 
map) {
 
     public static Time getStandaloneClusterStartupPeriodTime(Configuration 
configuration) {
         final Time timeout;
-        long standaloneClusterStartupPeriodTime =
+        Duration standaloneClusterStartupPeriodTime =
                 
configuration.get(ResourceManagerOptions.STANDALONE_CLUSTER_STARTUP_PERIOD_TIME);
-        if (standaloneClusterStartupPeriodTime >= 0) {
-            timeout = Time.milliseconds(standaloneClusterStartupPeriodTime);
+        if (standaloneClusterStartupPeriodTime != null) {
+            timeout = Time.fromDuration(standaloneClusterStartupPeriodTime);

Review Comment:
   What happens if the configured value is negative? Can a Duration be negative?



-- 
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-unsubscr...@flink.apache.org

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

Reply via email to