You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by srowen <gi...@git.apache.org> on 2016/04/15 15:32:24 UTC
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
GitHub user srowen opened a pull request:
https://github.com/apache/spark/pull/12418
[SPARK-8393] [STREAMING] JavaStreamingContext#awaitTermination() throws non-declared InterruptedException
## What changes were proposed in this pull request?
`JavaStreamingContext.awaitTermination` methods should be declared as `@throws[IOException]` so that this exception can be handled in Java code. Note this is not just a doc change, but an API change, since now (in Java) the method has a checked exception to handle. All await-like methods in Java APIs behave this way, so seems worthwhile for 2.0.
## How was this patch tested?
Jenkins tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/srowen/spark SPARK-8393
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/12418.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 #12418
----
commit 8cfb3208d2919ee3563fde2634cb7ab95b2abeb8
Author: Sean Owen <so...@cloudera.com>
Date: 2016-04-15T13:05:19Z
JavaStreamingContext.awaitTermination methods should be declared as @throws[IOException] so that this exception can be handled in Java code.
----
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-211488970
@zsxwing throwing `InterruptedException`? I would think just the "await" methods would throw that. I think there might be a more general issue with `SparkException`?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-212023778
@srowen make sense. LGTM.
By the way, I realized one problem of this method. There are two cases that the `await**` method will throw `InterruptedException`:
1. The current thread is interrupted
2. ContextWaiter is notified with `InterruptedException`
What do you think about this problem?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-212532149
:+1: for merging this one. @srowen could you update `@throws[IOException]` to `@throws[InterruptedException]` in the PR description?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-211817268
I suppose this argument holds for lots of methods and lots of exceptions. For example, you can't catch `SparkException` from Java for any API method. It will still be thrown, and can be handled by catching for `Exception` and in many cases that's all the driver program would care to do anyway -- trap it and fail or move on.
"await" methods in the API however use an exception as part of their control flow. Whether or not it timed out depends on whether it returned with or without throwing. So it's fairly important to be able to handle that exception in the Java caller. I don't know if the same applies for general actions; they can throw `InterruptedException` but would I want to handle that?
While I think this change is right, I'm also wondering whether there are logically related changes that must also be made for consistency, that are something short of "annotating every single Java API method"! WDYT?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-211490038
`DAGScheduler.runJob` calls `Awaitable.ready` which will throw `InterruptedException`. In general, actions will call this method. Therefore, they will also throw `InterruptedException`.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by huitseeker <gi...@git.apache.org>.
Github user huitseeker commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-210867136
LGTM !
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-210485690
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/55928/
Test PASSed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-212841238
Merged to master
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by srowen <gi...@git.apache.org>.
Github user srowen closed the pull request at:
https://github.com/apache/spark/pull/12418
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by zsxwing <gi...@git.apache.org>.
Github user zsxwing commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-211487345
I think all action methods of Java RDD have this issue. Is it worth to only fix this special one?
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-210485504
**[Test build #55928 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/55928/consoleFull)** for PR 12418 at commit [`8cfb320`](https://github.com/apache/spark/commit/8cfb3208d2919ee3563fde2634cb7ab95b2abeb8).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-210485687
Merged build finished. Test PASSed.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/12418#discussion_r59874007
--- Diff: examples/src/main/java/org/apache/spark/examples/streaming/JavaDirectKafkaWordCount.java ---
@@ -64,8 +66,8 @@ public static void main(String[] args) {
SparkConf sparkConf = new SparkConf().setAppName("JavaDirectKafkaWordCount");
JavaStreamingContext jssc = new JavaStreamingContext(sparkConf, Durations.seconds(2));
- HashSet<String> topicsSet = new HashSet<>(Arrays.asList(topics.split(",")));
- HashMap<String, String> kafkaParams = new HashMap<>();
+ Set<String> topicsSet = new HashSet<>(Arrays.asList(topics.split(",")));
--- End diff --
OK, just touched up this unrelated point while I was here.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-210466689
**[Test build #55928 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/55928/consoleFull)** for PR 12418 at commit [`8cfb320`](https://github.com/apache/spark/commit/8cfb3208d2919ee3563fde2634cb7ab95b2abeb8).
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-8393] [STREAMING] JavaStreamingContext#...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/12418#issuecomment-212376795
@zsxwing I made a mistake in my description above. Actually the return value determines whether timeout occurred. You are correct that really `InterruptedException` only occurs when the thread is interrupted while waiting, not on timeout. That still seems like something one might want to catch, and JDK "await" methods throw this exception. These are the only await methods I can see in the Java-specific API. I still think this is worth merging, but really this is a complex API question.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org