You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by sarutak <gi...@git.apache.org> on 2014/08/21 07:44:10 UTC
[GitHub] spark pull request: [SPARK-3171] Don't print meaningless informati...
GitHub user sarutak opened a pull request:
https://github.com/apache/spark/pull/2078
[SPARK-3171] Don't print meaningless information of SelectionKey
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sarutak/spark SPARK-3171
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/2078.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 #2078
----
commit f974132ef705b2228a62e5cb6f782870dc4f68f0
Author: Kousuke Saruta <sa...@oss.nttdata.co.jp>
Date: 2014-08-21T05:43:20Z
Improve error message when facing CancelledKeyException
----
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-52880987
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19035/consoleFull) for PR 2078 at commit [`f974132`](https://github.com/apache/spark/commit/f974132ef705b2228a62e5cb6f782870dc4f68f0).
* This patch merges cleanly.
---
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-3171] Don't print meaningless informati...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-55678561
Can anyone review this if you have time?
---
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-3171] Don't print meaningless informati...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-55364261
@mridulm @JoshRosen Does this PR look good to you guys?
---
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-3171] Don't print meaningless informati...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53151994
Thanks @mridulm .
The answer I want is why selector.select throws CancelledKeyException even through JavaDoc doesn't say select() throws the exception.
Before, there was a bug that Selector#select throws CancelledKeyException but it's already resolved.
You can check that here (http://www.oracle.com/technetwork/java/javase/releasenotes-138306.html , Bug ID is 4729342)
If you are worried about there is still bug, we should leave comment why enclosing selector.select by try block or log it may be JDK bug when throwing CancelledKeyException from selector.select.
---
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-3171] Don't print meaningless informati...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-52933408
@mridulm I don't think, It's not loud to print information that a key is related to which host:port.
Printing only key(.toString()) is rather loud because it's not helpful information (key.toString is <class name>@<hashcode>).
I think, printing remote host:port is needed information in the case of trouble shooting.
Needed information should be logged at the time it's needed.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-52932490
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19054/consoleFull) for PR 2078 at commit [`4dc6050`](https://github.com/apache/spark/commit/4dc60507009b5b2e0364a4f8f18c31b251581ddc).
* This patch **fails** unit 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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-55484809
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20263/consoleFull) for PR 2078 at commit [`df5ca5c`](https://github.com/apache/spark/commit/df5ca5c69a38cbc36df847c48d913adcb832d4f0).
* This patch merges cleanly.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-55514287
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20292/consoleFull) for PR 2078 at commit [`2419304`](https://github.com/apache/spark/commit/24193049686d267d24150bd362ffe14e8645459d).
* This patch **passes** unit tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `class RatingDeserializer(FramedSerializer):`
* ` class Encoder[T <: NativeType](columnType: NativeColumnType[T]) extends compression.Encoder[T] `
* ` class Encoder[T <: NativeType](columnType: NativeColumnType[T]) extends compression.Encoder[T] `
* ` class Encoder[T <: NativeType](columnType: NativeColumnType[T]) extends compression.Encoder[T] `
* ` class Encoder extends compression.Encoder[IntegerType.type] `
* ` class Decoder(buffer: ByteBuffer, columnType: NativeColumnType[IntegerType.type])`
* ` class Encoder extends compression.Encoder[LongType.type] `
* ` class Decoder(buffer: ByteBuffer, columnType: NativeColumnType[LongType.type])`
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-54059387
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19561/consoleFull) for PR 2078 at commit [`000b941`](https://github.com/apache/spark/commit/000b941db18f462c06d1bf3106898e8cc5f4383b).
* This patch merges cleanly.
---
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-3171] Don't print meaningless informati...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53150053
If you do want to get to host port in case logs are noisy (happens!),
ensure you retrieve it with robust code.
This pr can cause npe's
On 23-Aug-2014 4:42 pm, "Kousuke Saruta" <no...@github.com> wrote:
> Regarding logging -
> host:port is insufficient to debug (and as I mentioned above, you will get
> NPE for the log messages in > this PR depending on the key's state) - you
> actually need the key instance correlated across log messages to debug
> issues.
> To find host:port for a key, it is logged elsewhere in the code iirc (and
> we grep by instance id).
>
> Is this true? I cannot found log message which shows the key and
> corresponding host:port.
> In ConnectionManager, when accepting connection and connecting to another
> host, host:port is logged so logging host:port when error occurred is
> helpful.
> Even if we can found host:port for a key, I think, it's good to be able to
> find host:port directly.
> I think, "be able to debug" is not "easy to debug".
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/2078#issuecomment-53149962>.
>
---
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-3171] Don't print meaningless informati...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53154886
I am not on mac - and have seen this on solaris and linux (not in ctx of spark); so the issue is not mac specific unfortunately (though it might also be on mac !).
Btw, we cannot assume that channels are readable/writable when they are in selector loop - it also includes channels which we have registered to connect, might not be connectable (remote host/rack gone, dns issues, etc).
Simple example where it fails : connect socket, and before connect completes, try to get remote address - it will return null. This specific example is reasonably beneign, we dont get NPE - just garbage in logs. (which is also the reason why we should always log the selection key directly)
---
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-3171] Don't print meaningless informati...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53149962
> Regarding logging -
> host:port is insufficient to debug (and as I mentioned above, you will get NPE for the log messages in > this PR depending on the key's state) - you actually need the key instance correlated across log messages to debug issues.
> To find host:port for a key, it is logged elsewhere in the code iirc (and we grep by instance id).
Is this true? I cannot found log message which shows the key and corresponding host:port.
In ConnectionManager, when accepting connection and connecting to another host, host:port is logged so logging host:port when error occurred is helpful.
Even if we can found host:port for a key, I think, it's good to be able to find host:port directly.
I think, "be able to debug" is not "easy to debug".
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-52932355
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19054/consoleFull) for PR 2078 at commit [`4dc6050`](https://github.com/apache/spark/commit/4dc60507009b5b2e0364a4f8f18c31b251581ddc).
* This patch merges cleanly.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-54066507
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19561/consoleFull) for PR 2078 at commit [`000b941`](https://github.com/apache/spark/commit/000b941db18f462c06d1bf3106898e8cc5f4383b).
* This patch **passes** unit 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-3171] Don't print meaningless informati...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-52960988
Regarding logging -
host:port is insufficient to debug (and as I mentioned above, you will get NPE for the log messages in this PR depending on the key's state) - you actually need the key instance correlated across log messages to debug issues.
To find host:port for a key, it is logged elsewhere in the code iirc (and we grep by instance id).
Regarding changes to try/catch, etc : it looks regressive - we spent quite a while making this class reasonably bugfree early last year - I have not kept up to date with modifications to this class, but this PR will break functionality,
---
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-3171] Don't print meaningless informati...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53150028
I removed try / catch because selector.select never throws CancelledKeyException.
If it can be regression, could you show me how CancelledKeyException is thrown from selector.select?
---
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-3171] Don't print meaningless informati...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-52920194
You can potentially have NPE's in some of these log message changes proposed.
The reason to keep the information minimal was to ensure we get atleast some information without complicating the log messaging.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53881683
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19473/consoleFull) for PR 2078 at commit [`5f2182d`](https://github.com/apache/spark/commit/5f2182d34b3325f06ef0dddafdfcceb8bf17bb08).
* This patch merges cleanly.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-55088015
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20094/consoleFull) for PR 2078 at commit [`4e9e7d4`](https://github.com/apache/spark/commit/4e9e7d4f55c397033f36761cacc1b9b6dec04a26).
* This patch **passes** unit 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-3171] Don't print meaningless informati...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53184688
Thanks @mridulm , @JoshRosen !
So, how about the solution as follows?
* Leave try / catch for CancelledKeyException which can be thrown from selector.select()
* Separate function to get the pair of remote address and key from logging statements.
* Just in case, protect the logic to get remote address for NPE.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-55513251
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20292/consoleFull) for PR 2078 at commit [`2419304`](https://github.com/apache/spark/commit/24193049686d267d24150bd362ffe14e8645459d).
* This patch merges cleanly.
---
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-3171] Don't print meaningless informati...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53153441
Why it happens is anyone's guess, I know it happens since fairly the very beginning when I started using nio in 1.4.x; and continues till today. If you search online, you will see a lot of people still hit it (and most of them are not on 1.4.x).
I added this codepath since selector thread was dying due to this issue.
Btw, if you actually look at the evaluation of the bug, you will notice that some workarounds are suggested - which are not applicable to a moderately multi-threaded app like spark (we dont know 'when' we will be done with a key !).
---
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-3171] Don't print meaningless informati...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak closed the pull request at:
https://github.com/apache/spark/pull/2078
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-52951862
**Tests timed out** after a configured wait of `120m`.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-52884177
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19035/consoleFull) for PR 2078 at commit [`f974132`](https://github.com/apache/spark/commit/f974132ef705b2228a62e5cb6f782870dc4f68f0).
* This patch **passes** unit 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-3171] Don't print meaningless informati...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53150077
It can and it does, if key was cancelled
On 23-Aug-2014 4:46 pm, "Kousuke Saruta" <no...@github.com> wrote:
> I removed try / catch because selector.select never throws
> CancelledKeyException.
> If it can be regression, could you show me how CancelledKeyException is
> thrown from selector.select?
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/2078#issuecomment-53150028>.
>
---
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-3171] Don't print meaningless informati...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-54701484
Jenkins, retest this please.
---
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-3171] Don't print meaningless informati...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53154553
I don't mind handling NPE but I wonder why / where / how NPE is thrown.
At least, JavaDoc doesn't say SelectionKey#channel, SocketChannel#socket return null.
And after key is cancelled, key.channel still return non-null object.
And I found, the bug of throwing CancelledKeyException may be for MacOSX.
https://java.net/projects/macosx-port/lists/issues/archive/2011-10/message/186
O.K. I agree to leave try / catch but I think, we should leave comment why select should be enclosed try block.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-54497354
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19786/consoleFull) for PR 2078 at commit [`694d8c0`](https://github.com/apache/spark/commit/694d8c0591a86429542c84b8ca1d08523df5dd9d).
* This patch merges cleanly.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-55082131
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20094/consoleFull) for PR 2078 at commit [`4e9e7d4`](https://github.com/apache/spark/commit/4e9e7d4f55c397033f36761cacc1b9b6dec04a26).
* This patch merges cleanly.
---
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-3171] Don't print meaningless informati...
Posted by sarutak <gi...@git.apache.org>.
Github user sarutak commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-55080894
@rxin I've rebased.
---
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-3171] Don't print meaningless informati...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53155180
/CC @JoshRosen since you looked at ConnectionManager and Connection recently.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-54698334
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19867/consoleFull) for PR 2078 at commit [`694d8c0`](https://github.com/apache/spark/commit/694d8c0591a86429542c84b8ca1d08523df5dd9d).
* This patch **fails** unit 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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53197438
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19125/consoleFull) for PR 2078 at commit [`078a908`](https://github.com/apache/spark/commit/078a908b37d0fe6860e7f8823b30372739f3599b).
* This patch merges cleanly.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-54695098
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19867/consoleFull) for PR 2078 at commit [`694d8c0`](https://github.com/apache/spark/commit/694d8c0591a86429542c84b8ca1d08523df5dd9d).
* This patch merges cleanly.
---
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-3171] Don't print meaningless informati...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53176045
My initial thoughts: the ConnectionManager is a complex piece of code that's been the source of many bugs, so I'd like to be conservative when it comes to changing it.
If there's even a slight chance that the `CancelledKeyException` might be thrown, even if it's due to a platform bug, I'd like to keep the code that c3a4bc3ae0fe04d1f40848e9ac8169f264e43880 removed.
I like @mridulm's suggestion of making a separate helper function for this logging. Feel free to summarize some of this discussion in comments, too, including the rationale for why remoteAddress won't NPE (if it was complicated enough to provoke this much discussion, then it's worth documenting).
---
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-3171] Don't print meaningless informati...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53184900
Sounds good, tux !
On 24-Aug-2014 2:28 pm, "Kousuke Saruta" <no...@github.com> wrote:
> Thanks @mridulm <https://github.com/mridulm> , @JoshRosen
> <https://github.com/JoshRosen> !
> So, how about the solution as follows?
>
> - Leave try / catch for CancelledKeyException which can be thrown from
> selector.select()
> - Separate function to get the pair of remote address and key from
> logging statements.
> - Just in case, protect the logic to get remote address for NPE.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/2078#issuecomment-53184688>.
>
---
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-3171] Don't print meaningless informati...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53155129
Looking at the code :
'''
val remoteAddress = key.channel.asInstanceOf[SocketChannel].socket.getRemoteSocketAddress
'''
I agree, key.channel is document to always return non null value; SocketChannel.socket is implemented to create new socket and return that in case socket happens to be null.
And so looks like NPE is not possible.
Returned socket address can be null and making log message useless; but atleast thankfully we wont barf out with an exception !
Given this is for logging, make it a addressInfo String (in a method possibly since this is used in multiple places) and populate it with:
a) getRemoteSocketAddress (null should be fine I guess).
b) key.toString (would include key identifier to help debug lifecycle of a selection key).
Possibly other info which might be relevant ... and use this instead of remoteAddress in the code.
Given that spark logs sometimes go upto 60+ GB for some of our jobs, I can see the value in having the socket addresses along with other state transition information while debugging a problem (instead of having to grep to find this info !)
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-53199027
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19125/consoleFull) for PR 2078 at commit [`078a908`](https://github.com/apache/spark/commit/078a908b37d0fe6860e7f8823b30372739f3599b).
* This patch **passes** unit 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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-54508054
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19786/consoleFull) for PR 2078 at commit [`694d8c0`](https://github.com/apache/spark/commit/694d8c0591a86429542c84b8ca1d08523df5dd9d).
* This patch **passes** unit 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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-52934588
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19056/consoleFull) for PR 2078 at commit [`5db30f0`](https://github.com/apache/spark/commit/5db30f09d1f6001e9bcd74a3c80427da1c7694af).
* This patch merges cleanly.
---
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-3171] Don't print meaningless informati...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-55079128
This no longer merges with master. Do you mind rebasing it? I moved the files into the o.a.s.network.nio package.
---
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-3171] Don't print meaningless informati...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2078#issuecomment-55485996
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/20263/consoleFull) for PR 2078 at commit [`df5ca5c`](https://github.com/apache/spark/commit/df5ca5c69a38cbc36df847c48d913adcb832d4f0).
* This patch **passes** unit tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `class JavaSparkContext(val sc: SparkContext)`
* `class JavaStreamingContext(val ssc: StreamingContext) extends Closeable `
---
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