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 2015/02/13 18:54:05 UTC
[GitHub] flink pull request: [FLINK-1543] Adds better exception handling in...
GitHub user tillrohrmann opened a pull request:
https://github.com/apache/flink/pull/394
[FLINK-1543] Adds better exception handling in actors
Adds try and catch blocks around exception throwing methods in the JobManager and TaskManager. This PR is based on #384.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tillrohrmann/flink akkaExceptions
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/394.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 #394
----
commit bfeac6becc00f3bb02379ff448aefab95548082c
Author: Till Rohrmann <tr...@apache.org>
Date: 2015-02-10T11:21:33Z
[FLINK-1508] [runtime] Removes AkkaUtil.ask and replaces respective calls with explicit future handling.
Removes blocking calls for ActorRef retrieval in actors.
This closes #384.
commit 86976bb0feab8abb43ab302a61294157cd1fe35a
Author: Till Rohrmann <tr...@apache.org>
Date: 2015-02-12T10:49:09Z
[FLINK-1543] [runtime] Adds try and catch blocks around all method calls from an actor's receive method which can throw an exception. Sets the StoppingSupervisorStrategy as default for all guardians.
This closes #394.
----
---
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-1543] Adds better exception handling in...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/flink/pull/394#discussion_r24689680
--- Diff: flink-clients/src/main/java/org/apache/flink/client/CliFrontend.java ---
@@ -517,67 +521,83 @@ protected int list(String[] args) {
return 1;
}
- Iterable<ExecutionGraph> jobs = AkkaUtils.<RunningJobs>ask(jobManager,
- RequestRunningJobs$.MODULE$, getAkkaTimeout()).asJavaIterable();
+ final Future<Object> response = Patterns.ask(jobManager,
+ JobManagerMessages.getRequestRunningJobs(), new Timeout(getAkkaTimeout()));
- ArrayList<ExecutionGraph> runningJobs = null;
- ArrayList<ExecutionGraph> scheduledJobs = null;
- if (running) {
- runningJobs = new ArrayList<ExecutionGraph>();
- }
- if (scheduled) {
- scheduledJobs = new ArrayList<ExecutionGraph>();
+ Object result = null;
+
+ try{
+ result = Await.result(response, getAkkaTimeout());
+ } catch (Exception exception) {
+ throw new IOException("Could not retrieve running jobs from job manager.",
+ exception);
}
-
- for (ExecutionGraph rj : jobs) {
-
- if (running && rj.getState().equals(JobStatus.RUNNING)) {
- runningJobs.add(rj);
+
+ if(!(result instanceof RunningJobs)){
--- End diff --
More like nit style change. Most code in Flink add extra space between 'if' and the '(' and also a space before '{'.
---
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-1543] Adds better exception handling in...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/394
---
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-1543] Adds better exception handling in...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/flink/pull/394#discussion_r24690375
--- Diff: flink-runtime/src/test/java/org/apache/flink/runtime/taskmanager/TaskManagerTest.java ---
@@ -486,7 +489,8 @@ protected void run() {
public void onReceive(Object message) throws Exception {
if(message instanceof RegistrationMessages.RegisterTaskManager){
final InstanceID iid = new InstanceID();
- getSender().tell(new RegistrationMessages.AcknowledgeRegistration(iid, -1),
+ getSender().tell(new RegistrationMessages.AcknowledgeRegistration(iid, -1,
+ Option.<ActorRef>apply(null)),
--- End diff --
Just curious, why not just pass None instead of {{Option.<ActorRef>apply(null)}} ?
---
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-1543] Adds better exception handling in...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the pull request:
https://github.com/apache/flink/pull/394#issuecomment-74385078
@hsaputra, sorry I did not see your comments before I committed the PR. I started merging it yesterday but it always failed because of some minor issues on Travis. Therefore, I only looked at the Travis results. Won't happen again.
You're right that the extra spaces are missing. Good catch. I'll add them with my next commit.
---
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-1543] Adds better exception handling in...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/394#discussion_r24693247
--- Diff: flink-runtime/src/test/java/org/apache/flink/runtime/taskmanager/TaskManagerTest.java ---
@@ -486,7 +489,8 @@ protected void run() {
public void onReceive(Object message) throws Exception {
if(message instanceof RegistrationMessages.RegisterTaskManager){
final InstanceID iid = new InstanceID();
- getSender().tell(new RegistrationMessages.AcknowledgeRegistration(iid, -1),
+ getSender().tell(new RegistrationMessages.AcknowledgeRegistration(iid, -1,
+ Option.<ActorRef>apply(null)),
--- End diff --
I think that `apply(null)` is actually a `Some`, not a `None`.
---
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-1543] Adds better exception handling in...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/394#discussion_r24716021
--- Diff: flink-runtime/src/test/java/org/apache/flink/runtime/taskmanager/TaskManagerTest.java ---
@@ -486,7 +489,8 @@ protected void run() {
public void onReceive(Object message) throws Exception {
if(message instanceof RegistrationMessages.RegisterTaskManager){
final InstanceID iid = new InstanceID();
- getSender().tell(new RegistrationMessages.AcknowledgeRegistration(iid, -1),
+ getSender().tell(new RegistrationMessages.AcknowledgeRegistration(iid, -1,
+ Option.<ActorRef>apply(null)),
--- End diff --
That was the only way I found to create a ```None``` with type parameter ```ActorRef``` in Java. ```None``` itself in Java has no type parameter.
---
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-1543] Adds better exception handling in...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/394#issuecomment-74311404
+1 Nice work and a critical fix. Prevents the root actors from failing on user code problems.
---
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-1543] Adds better exception handling in...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the pull request:
https://github.com/apache/flink/pull/394#issuecomment-74388302
No worries, thanks for replying to my concern =)
---
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-1543] Adds better exception handling in...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/flink/pull/394#discussion_r24689710
--- Diff: flink-clients/src/main/java/org/apache/flink/client/CliFrontend.java ---
@@ -517,67 +521,83 @@ protected int list(String[] args) {
return 1;
}
- Iterable<ExecutionGraph> jobs = AkkaUtils.<RunningJobs>ask(jobManager,
- RequestRunningJobs$.MODULE$, getAkkaTimeout()).asJavaIterable();
+ final Future<Object> response = Patterns.ask(jobManager,
+ JobManagerMessages.getRequestRunningJobs(), new Timeout(getAkkaTimeout()));
- ArrayList<ExecutionGraph> runningJobs = null;
- ArrayList<ExecutionGraph> scheduledJobs = null;
- if (running) {
- runningJobs = new ArrayList<ExecutionGraph>();
- }
- if (scheduled) {
- scheduledJobs = new ArrayList<ExecutionGraph>();
+ Object result = null;
+
+ try{
--- End diff --
More like nit style change. Most code in Flink add a space before '{'.
---
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-1543] Adds better exception handling in...
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the pull request:
https://github.com/apache/flink/pull/394#issuecomment-74384108
@tillrohrmann I had concern about style and question about value about parameter passed.
Would be nice to address those before committing.
---
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.
---