Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FLINK-2523: making the task cancellation interval configurable. #1546

Closed
wants to merge 1 commit into from
Closed

FLINK-2523: making the task cancellation interval configurable. #1546

wants to merge 1 commit into from
+96 −34

Conversation

kl0u
Copy link
Contributor

@kl0u kl0u commented Jan 25, 2016

FLINK-2523: Makes the task cancellation interval configurable.

rmetzger
rmetzger reviewed Jan 25, 2016
View reviewed changes
...s/flink-examples-batch/src/main/java/org/apache/flink/examples/java/wordcount/WordCount.java
@@ -60,6 +60,7 @@ public static void main(String[] args) throws Exception {

// set up the execution environment
final ExecutionEnvironment env = ExecutionEnvironment.getExecutionEnvironment();
env.getConfig().setTaskCancellationDelay(40000);
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that's an accidental commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right.

@kl0u kl0u force-pushed the task_cancellation branch 2 times, most recently from f53a88e to 17df644 Compare January 25, 2016 14:05
@StephanEwen
Copy link
Contributor

Thank you for this patch. The idea is good, but I think it needs a bit of improvement:

  • The value is pulled in a fairly complicated way though all the layers and abstractions. That is not really necessary, as soon as it is in the ExecutionConfig, the Task can access the values. No need to change the TaskDeploymentDescriptior, JobGraph, ExecutionGraph, etc. This probably needs changes in 2 files (otherwise, our abstractions are messed up).
  • taskCancellationDelay is not a very intuitive name, in my opinion. It suggests that cancellation is delayed, which is not the case. Tasks are actually canceled a single time, the blocked threads are only repeatedly interrupted.
  • We should not try to show off such parameters in the examples. The examples need to stay simple, or we confuse people with parameters they need usually not worry about.

@tillrohrmann
Copy link
Contributor

The JobManagerITCase is completely commented out. I guess this is a testing artifact.

@kl0u
Copy link
Contributor Author

kl0u commented Jan 25, 2016

Hi @StephanEwen ,

Could you please elaborate more on how you think that the ExecutionConfig could be accessed differently by the Task?

Thanks!

@StephanEwen
Copy link
Contributor

Ah, true, the ExecutionConfig is currently grabed in a pretty non-transparent way from the JobConfig.

I think it would be a good refactoring to pass the ExecutionConfig directly via the TaskDeploymentDescriptor. Then the canceling interval can be passed simply through there.

@kl0u kl0u force-pushed the task_cancellation branch 2 times, most recently from 0e23d1d to c43e9bd Compare January 27, 2016 16:21
StephanEwen
StephanEwen reviewed Jan 28, 2016
View reviewed changes
flink-runtime/src/main/java/org/apache/flink/runtime/deployment/TaskDeploymentDescriptor.java
@@ -93,24 +94,28 @@
private final SerializedValue<StateHandle<?>> operatorState;

private long recoveryTimestamp;


/** The interval (in millis) between consecutive task cancellation retries. */
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is probably still from an earlier version of the code

@StephanEwen
Copy link
Contributor

This is going into a good direction. Two suggestions:

  • Since you already have the ExecutionConfig in the Task, I would put it into the Environment from there. Then we could also get rid of the strange way of reading the ExecutionConfig in the AbstractInvokable
  • The ExecutionConfig should be directly attached to the JobGraph, rather than written to the job config and read from there.

@kl0u kl0u closed this Feb 4, 2016
@kl0u kl0u deleted the task_cancellation branch June 20, 2016 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
component=Runtime/Coordination
Projects
None yet
Milestone
No milestone
4 participants
@kl0u @StephanEwen @tillrohrmann @rmetzger