[FLINK-32023][API / DataStream] Support negative Duration for special usage, such as -1ms for execution.buffer-timeout #22560

Myracle · 2023-05-10T07:56:02Z

What is the purpose of the change

Support negative Duration for special usage, such as -1ms for execution.buffer-timeout

Brief change log

Support to parse the negative sign in TimeUtils's method parseDuration.

Verifying this change

This change added tests and can be verified as follows:

Added test testParseDurationWithNegativeNumber in TimeUtilsTest.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)

flinkbot · 2023-05-10T08:05:39Z

CI report:

40636d9 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

1996fanrui

Hi @Myracle , thanks for your contribution.

I see the CI fails, please run mvn spotless:apply before commiting.

flink-core/src/main/java/org/apache/flink/util/TimeUtils.java

flink-core/src/test/java/org/apache/flink/util/TimeUtilsTest.java

zentol · 2023-05-10T11:02:11Z

Could we not do this? This now break assumptions that every other component can make in that configured durations are always positive.

Either use Configuration#getOptional (+ adding it by default to flink-conf.yaml) or add a boolean option to disable the timeout.

Myracle · 2023-05-10T12:08:20Z

@zentol I agree with you for that the change may affect other usages. What about adding a boolean config named execution.buffer-timeout.enabled which default value is true. When it is false, flushing only when the output buffer is full. At the same time, the execution.buffer-timeout's value will be ignored. cc @1996fanrui

1996fanrui · 2023-05-10T13:08:51Z

Thanks @zentol 's feedback here.

What about adding a boolean config named execution.buffer-timeout.enabled which default value is true. When it is false, flushing only when the output buffer is full. At the same time, the execution.buffer-timeout's value will be ignored.

Sounds make sense.

1996fanrui · 2023-05-16T03:07:54Z

flink-core/src/main/java/org/apache/flink/configuration/ExecutionOptions.java

                            "Tells if we should use compression for the state snapshot data or not");

+    public static final ConfigOption<Boolean> BUFFER_TIMEOUT_ENABLED =
+            ConfigOptions.key("execution.buffer-timeout.enabled")


FLINK-29372 limited that the option key cannot be a prefix of other options, however execution.buffer-timeout is the prefix of execution.buffer-timeout.enabled. So the CI fails.

Hi @zentol , could we update the option key for BUFFER_TIMEOUT and mark the "execution.buffer-timeout" as the DeprecatedKey? How about update it to execution.buffer-timeout.interval ? Or do you have any suggestions here? Looking forward to your opinion.

How about changing the config execution.buffer-timeout.enabled to execution.flush-on-buffer-full?

Hi @Myracle , thanks for your feedback.

I see too many related option names that all have the same prefix but different suffixes. From the user's point of view, the same prefix also clearly tells the user that these options are related. Therefore, when FLINK-29372 restricts the option prefix, it also adds suffixes to many option names that do not conform to the specification.

So I prefer keep the prefixes the same for these 2 options. In order to the reasonable of this fix, I look forward to more feedback from community.

Hi @wanglijie95 @reswqa , would you mind take a look this PR in your free time? thanks a lot.

Rename it to execution.buffer-timeout.interval and mark the old as deprecated key is ok from my side. But I want to hear @zentol's thoughts.

BTW, I'm not sure do we need to start a discussion about this changes of configuration in the mailing list.

+1 for name execution.buffer-timeout.interval

Rename it to execution.buffer-timeout.interval. @zentol Would you like to give your thought? Thanks.

execution.buffer-timeout.interval and execution.buffer-timeout.enabled sound fine to me.

flink-core/src/main/java/org/apache/flink/configuration/ExecutionOptions.java

1996fanrui

Thanks @Myracle 's update, LGTM.

1996fanrui · 2023-05-26T06:24:56Z

I will merge this PR next Monday if no other comments.

Hi @Myracle , could you help squash all commits? thanks~

…nabled to flush only when the output buffer is full.

1996fanrui · 2023-05-29T02:26:52Z

@flinkbot run azure

1996fanrui · 2023-05-29T06:06:05Z

Thanks everyone who discussed here, and @Myracle 's contribution, merging.

1996fanrui self-requested a review May 10, 2023 10:07

flinkbot added the component=API/DataStream label May 10, 2023

1996fanrui reviewed May 10, 2023

View reviewed changes

flink-core/src/main/java/org/apache/flink/util/TimeUtils.java Outdated Show resolved Hide resolved

flink-core/src/test/java/org/apache/flink/util/TimeUtilsTest.java Outdated Show resolved Hide resolved

Myracle force-pushed the FLINK-32023-support-negative-execution.buffer-timeout branch from 507ea2e to a3ce22d Compare May 15, 2023 06:22

1996fanrui reviewed May 16, 2023

View reviewed changes

1996fanrui requested changes May 24, 2023

View reviewed changes

flink-core/src/main/java/org/apache/flink/configuration/ExecutionOptions.java Outdated Show resolved Hide resolved

Myracle force-pushed the FLINK-32023-support-negative-execution.buffer-timeout branch from 57d43fc to a1f6d45 Compare May 24, 2023 07:08

1996fanrui approved these changes May 25, 2023

View reviewed changes

[FLINK-32023][API / DataStream] Add config execution.buffer-timeout.e…

40636d9

…nabled to flush only when the output buffer is full.

Myracle force-pushed the FLINK-32023-support-negative-execution.buffer-timeout branch from a1f6d45 to 40636d9 Compare May 26, 2023 12:15

1996fanrui merged commit 522ff83 into apache:master May 29, 2023

[FLINK-32023][API / DataStream] Support negative Duration for special usage, such as -1ms for execution.buffer-timeout #22560

[FLINK-32023][API / DataStream] Support negative Duration for special usage, such as -1ms for execution.buffer-timeout #22560

Uh oh!

Conversation

Myracle commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

flinkbot commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

1996fanrui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zentol commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Myracle commented May 10, 2023

Uh oh!

1996fanrui commented May 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

1996fanrui left a comment

Choose a reason for hiding this comment

Uh oh!

1996fanrui commented May 26, 2023

Uh oh!

1996fanrui commented May 29, 2023

Uh oh!

1996fanrui commented May 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Myracle commented May 10, 2023 •

edited

Loading

flinkbot commented May 10, 2023 •

edited

Loading

zentol commented May 10, 2023 •

edited

Loading