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