You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tillrohrmann <gi...@git.apache.org> on 2016/02/09 18:54:07 UTC
[GitHub] flink pull request: [FLINK-3260] [runtime] Enforce terminal state ...
GitHub user tillrohrmann opened a pull request:
https://github.com/apache/flink/pull/1613
[FLINK-3260] [runtime] Enforce terminal state of Executions
This commit fixes the problem that Executions could leave their terminal state
FINISHED to transition to FAILED. Such a transition will be propagated to the
ExecutionGraph where it entails JobStatus changes. Since the Execution already
reached a terminal state, it should not again affect the ExecutionGraph. This
can lead to an inconsistent state in case of a restart where the old Executions
get disassociated from the ExecutionGraph.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tillrohrmann/flink fixCallbacks
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1613.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1613
----
commit cb2a2fb6d3dde5e248e6153e849c8f07d241a10d
Author: Till Rohrmann <tr...@apache.org>
Date: 2016-02-09T09:30:12Z
[FLINK-3260] [runtime] Enforce terminal state of Executions
This commit fixes the problem that Executions could leave their terminal state
FINISHED to transition to FAILED. Such a transition will be propagated to the
ExecutionGraph where it entails JobStatus changes. Since the Execution already
reached a terminal state, it should not again affect the ExecutionGraph. This
can lead to an inconsistent state in case of a restart where the old Executions
get disassociated from the ExecutionGraph.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-3260] [runtime] Enforce terminal state ...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/1613#discussion_r52368138
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/Execution.java ---
@@ -107,7 +108,7 @@
private static final AtomicReferenceFieldUpdater<Execution, ExecutionState> STATE_UPDATER =
AtomicReferenceFieldUpdater.newUpdater(Execution.class, ExecutionState.class, "state");
- private static final Logger LOG = ExecutionGraph.LOG;
+ private static final Logger LOG = LoggerFactory.getLogger(Execution.class);
--- End diff --
Did this cause issues in this case? I originally set the logger to the ExecutionGraph logger to get all messages related to the execution and it changes in one log namespace. I always thought that makes searching the log easier.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-3260] [runtime] Enforce terminal state ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1613#discussion_r52441996
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/Execution.java ---
@@ -107,7 +108,7 @@
private static final AtomicReferenceFieldUpdater<Execution, ExecutionState> STATE_UPDATER =
AtomicReferenceFieldUpdater.newUpdater(Execution.class, ExecutionState.class, "state");
- private static final Logger LOG = ExecutionGraph.LOG;
+ private static final Logger LOG = LoggerFactory.getLogger(Execution.class);
--- End diff --
Ok, will revert it then.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-3260] [runtime] Enforce terminal state ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the pull request:
https://github.com/apache/flink/pull/1613#issuecomment-182277343
Thanks for the review @StephanEwen.
The problem is that the test case for the problem is inherently unstable on Travis, because it depends on timeouts. I don't really see yet how to make it reliably stable. If I can't find a solution, I will remove the test case. But I'll add another test case which verifies that one cannot fail a finished `Execution`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-3260] [runtime] Enforce terminal state ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1613
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-3260] [runtime] Enforce terminal state ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1613#discussion_r52432150
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/Execution.java ---
@@ -107,7 +108,7 @@
private static final AtomicReferenceFieldUpdater<Execution, ExecutionState> STATE_UPDATER =
AtomicReferenceFieldUpdater.newUpdater(Execution.class, ExecutionState.class, "state");
- private static final Logger LOG = ExecutionGraph.LOG;
+ private static final Logger LOG = LoggerFactory.getLogger(Execution.class);
--- End diff --
I have to admit that it confused me when parsing the log, because state changes of the `Execution` were reported from the `ExecutionGraph` logger. When I tried to pinpoint the exact location of the logging statement, I searched for the logging string in the `ExecutionGraph` but couldn't find it. But I also see your point. If you want to, I can revert the change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-3260] [runtime] Enforce terminal state ...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/1613#discussion_r52438500
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/Execution.java ---
@@ -107,7 +108,7 @@
private static final AtomicReferenceFieldUpdater<Execution, ExecutionState> STATE_UPDATER =
AtomicReferenceFieldUpdater.newUpdater(Execution.class, ExecutionState.class, "state");
- private static final Logger LOG = ExecutionGraph.LOG;
+ private static final Logger LOG = LoggerFactory.getLogger(Execution.class);
--- End diff --
I think that is to some extend matter of taste how fine grained you split the loggers. Also, actually, if you create loggers by class or by job.
If you don't mind keeping it, I would like to keep it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-3260] [runtime] Enforce terminal state ...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1613#issuecomment-182398407
Merging this...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-3260] [runtime] Enforce terminal state ...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/1613#issuecomment-182046481
Looks good, with one inline comment.
Otherwise, +1 to merge
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---