Skip to content

Conversation

StephanEwen
Copy link
Contributor

The TimerServiceOptions are in the wrong place, which prohibits generation of config docs. It also cause over-fragmentation of the options in the code base.

This PR moves the one option from that class to the TaskManagerOptions, as it relates to task execution. Other shutdown related options are in there already.

Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable.

Note: The generator can pick up ConfigOptions in any module so long as they are public (and the generator is configured to do so). This was recently implemented to support python/mesos/netty options that aren't in flink-core.

*/
public static final ConfigOption<Long> TASK_CANCELLATION_TIMEOUT_TIMERS = ConfigOptions
.key("task.cancellation.timeout.timers")
.defaultValue(7500L);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add deprecated key? (I'm not quite sure whether the previous option was part of a release)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - I just checked, it was in fact already part of 1.4 (I had falsely in my mind that it was new in 1.5)

@StephanEwen
Copy link
Contributor Author

Thanks for the heads up - nice to know that the generator also supports different projects now.

I am still leaning towards making this change, because all other shutdown options are already in the TaskManagerOptions, and this class just felt out of place in flink-streaming-java.

@StephanEwen StephanEwen force-pushed the timer_service_options branch from f5045b2 to 72e093b Compare February 2, 2018 15:00
StephanEwen added a commit to StephanEwen/flink that referenced this pull request Feb 4, 2018
StephanEwen added a commit to StephanEwen/flink that referenced this pull request Feb 14, 2018
StephanEwen added a commit to StephanEwen/flink that referenced this pull request Feb 14, 2018
StephanEwen added a commit to StephanEwen/flink that referenced this pull request Feb 15, 2018
StephanEwen added a commit to StephanEwen/flink that referenced this pull request Feb 16, 2018
@asfgit asfgit closed this in 85bfc07 Feb 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants