[GitHub] flink issue #5058: [FLINK-5465] [streaming] Wait for pending timer threads t...
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/5058 I was thinking about where to put those options and after all it was a choice between fragmentation and inconsistency. I considered `TaskManagerOptions` but it sounds like a wrong place, if you look what the other options in this class are about and it also feels like saying `Task` == `StreamTask` because timers are something that currently belongs to `streaming` imo. There are classes for configurations all over the place, so if you think this is a problem, maybe we should fix it in general and not on a per-case basis? I agree that we can change the key string. ---
[GitHub] flink issue #5058: [FLINK-5465] [streaming] Wait for pending timer threads t...
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/5058 I would like to make a few comments for followup: - I think `TimerServiceOptions` should not be an own class, we are getting a crazy fragmentation of options into classes (that have sometimes, like here, only one option defined). Let's merge these into the `TaskManagerOptions`. - The config key does not reflect the scheme in which all other config keys are defined. It reads like a name where the dot '.' is a work separator. The scheme in which all other config keys are defined is hierarchical, like a path in a nested config group/object structure. Think that the configuration is one huge JSON object, and the key is the dot path to the entry. Hence a key like `taskmanager.timers.shutdown-timeout` (or `taskmanager.timers.shutdown.timeout`, if we view `shutdown` as a full config group/object) would be in line with the resulting style. ---
[GitHub] flink issue #5058: [FLINK-5465] [streaming] Wait for pending timer threads t...
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/5058 Thanks for the reviews @aljoscha and @StephanEwen. Will merge this now. ---
[GitHub] flink issue #5058: [FLINK-5465] [streaming] Wait for pending timer threads t...
Github user aljoscha commented on the issue: https://github.com/apache/flink/pull/5058 +1 This LGTM now. ---
[GitHub] flink issue #5058: [FLINK-5465] [streaming] Wait for pending timer threads t...
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/5058 Implemented the configurable timeout, so that we can chose between the two options. ---
[GitHub] flink issue #5058: [FLINK-5465] [streaming] Wait for pending timer threads t...
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/5058 So which way can we agree upon? I feel like this is a setting that is very specific and hard to understand for the user. On the other hand, it is nice if we can adjust it by configuration in case it causes any problems. ---
[GitHub] flink issue #5058: [FLINK-5465] [streaming] Wait for pending timer threads t...
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/5058 I am actually leaning towards not having a grace period, too. Reason is that actually there are more than one method in the same code path that could also (theoretically) block forever. Should we remove the timeout? ---
[GitHub] flink issue #5058: [FLINK-5465] [streaming] Wait for pending timer threads t...
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/5058 Looks good all in all. I am torn between adding a configuration setting for the timer grace period and not having one. Seems like a setting that is unlikely to ever be used and just blows up the config space. On the other hand, magic numbers should be configurable... ---
[GitHub] flink issue #5058: [FLINK-5465] [streaming] Wait for pending timer threads t...
Github user StefanRRichter commented on the issue: https://github.com/apache/flink/pull/5058 CC @aljoscha ---
