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