You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by kishorvpatil <gi...@git.apache.org> on 2018/04/19 14:21:09 UTC
[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...
GitHub user kishorvpatil opened a pull request:
https://github.com/apache/storm/pull/2638
[STORM-3034] Adding exception stacktrace for executor failures in worker
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kishorvpatil/incubator-storm storm3034
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2638.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 #2638
----
commit c289c47c98307e20bdacbfe2f26d4655963c392f
Author: Kishor Patil <kp...@...>
Date: 2018-04-19T14:01:27Z
Adding exception stacktrace for executor failures in worker
----
---
[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2638#discussion_r182844665
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -361,6 +363,7 @@ public void run() {
Time.sleep(s);
}
} catch (Throwable t) {
+ LOG.info("Async loop Exception Stacktrace is: {} ", getStackTrace(t));
--- End diff --
Thanks. Wouldn't it make sense to add the throwable to the log statement there then, so both L370 and 372 print the exception? (I'm asking why we want a separate log for this stack trace)
---
[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2638#discussion_r182765656
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -109,6 +109,8 @@
import javax.security.auth.Subject;
+import static org.apache.commons.lang.exception.ExceptionUtils.*;
--- End diff --
nit: don't use *
---
[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2638#discussion_r182847223
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -109,6 +109,8 @@
import javax.security.auth.Subject;
+import static org.apache.commons.lang.exception.ExceptionUtils.*;
--- End diff --
Updated.
---
[GitHub] storm issue #2638: [STORM-3034] Adding exception stacktrace for executor fai...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the issue:
https://github.com/apache/storm/pull/2638
@srdo Moved the `LOG.error` before changing exception Cause.
---
[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2638#discussion_r182833344
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -109,6 +109,8 @@
import javax.security.auth.Subject;
+import static org.apache.commons.lang.exception.ExceptionUtils.*;
--- End diff --
+1. A little odd that checkstyle didn't catch this, since it's in the rules https://github.com/apache/storm/blob/master/storm-checkstyle/src/main/resources/storm/storm_checkstyle.xml#L82. @kishorvpatil could you check if maybe the number of allowed style violations for storm-client can be reduced?
---
[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2638#discussion_r182843808
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -361,6 +363,7 @@ public void run() {
Time.sleep(s);
}
} catch (Throwable t) {
+ LOG.info("Async loop Exception Stacktrace is: {} ", getStackTrace(t));
--- End diff --
The method can return at line 370 without reaching 372. We had instances where users could not understand root cause of the interrupt exception.
---
[GitHub] storm issue #2638: [STORM-3034] Adding exception stacktrace for executor fai...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2638
@kishorvpatil Can you elaborate a bit? I don't understand why `TimeoutException` would be wrapped in an `InterruptedException`? Where is the log line you posted coming from, a grep of Storm didn't turn anything up?
---
[GitHub] storm issue #2638: [STORM-3034] Adding exception stacktrace for executor fai...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the issue:
https://github.com/apache/storm/pull/2638
@srdo , What I mean is there are many instances where kafka is wrapping actual exceptions into `InterruptedException`. I am not sure why/what is the objective, but only way to understand the source of origin is to log the stack trace here.
There are many kafka classes ( `KafkaConsumer`, `KafkaConsumerCoordinator` etc. the wrap other exceptions into `InterruptedException` . Including [InterruptException.java](https://github.com/apache/kafka/blob/e31c0c9bdbad432bc21b583bd3c084f05323f642/clients/src/main/java/org/apache/kafka/common/errors/InterruptException.java#L39) where `InterrupedException` is created
---
[GitHub] storm issue #2638: [STORM-3034] Adding exception stacktrace for executor fai...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the issue:
https://github.com/apache/storm/pull/2638
@HeartSaVioR @srdo ., The issue in assuming that `InterruptedException` is only coming from external interruptions. Below is the actual example we noticed when Kafka Spout`TimeoutException` was wrapped in `InterruptedException`. Users could not identify where the exception was raised.
```
KafkaSpoutI_[39 39] [WARN] Expecting exception of class: class java.lang.InterruptedException, but exception chain only contains: (#<TimeoutException org.apache.kafka.common.errors.TimeoutException: Failed to get offsets by times in 40000 ms>)```
---
[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil closed the pull request at:
https://github.com/apache/storm/pull/2638
---
[GitHub] storm issue #2638: [STORM-3034] Adding exception stacktrace for executor fai...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2638
Sorry if I'm nitpicking you to death, but I don't think moving the log line is better. Now everyone gets an error level log, even if the exception is due to an interrupt. I'd prefer if we just include the exception in both this log statement and the one in the interrupt case.
---
[GitHub] storm issue #2638: [STORM-3034] Adding exception stacktrace for executor fai...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2638
@kishorvpatil Sorry to keep harping on this, but I still don't really understand how we're getting `InterruptedExceptions` that need to be logged at error level.
Could you give an example of a place in the Kafka code where an `InterruptedException` wraps another exception? I did a search of the Kafka repository for places where they throw `InterruptException`, and it looks like it's always just wrapping an `InterruptedException`? The `InterruptedException` constructor doesn't appear to allow you to wrap other exceptions either (i.e it has no `new InterruptedException(cause)` constructor)?
Do you know where the log line you posted is coming from? I initially thought it looked like a JUnit expected exception, but googling "but exception chain only contains" only pops up this PR and a log posted on this issue https://issues.apache.org/jira/browse/STORM-2873.
---
[GitHub] storm issue #2638: [STORM-3034] Adding exception stacktrace for executor fai...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2638
+1, though I'm not sure whether it is necessary to change the log level to error.
---
[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2638#discussion_r183233652
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -363,7 +358,7 @@ public void run() {
} catch (Throwable t) {
if (Utils.exceptionCauseIsInstanceOf(
InterruptedException.class, t)) {
- LOG.info("Async loop interrupted!");
+ LOG.error("Async loop interrupted!", t);
--- End diff --
Here we assume that it is not an error, so if my understanding is right, `info` level would make more sense. (I'd second @srdo.)
---
[GitHub] storm issue #2638: [STORM-3034] Adding exception stacktrace for executor fai...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the issue:
https://github.com/apache/storm/pull/2638
@srdo, You are right. This was exception I noticed on old internal version of the code package. It is clearly not necessary in 2.x latest code. Let me close this patch.
---
[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2638#discussion_r182833894
--- Diff: storm-client/src/jvm/org/apache/storm/utils/Utils.java ---
@@ -361,6 +363,7 @@ public void run() {
Time.sleep(s);
}
} catch (Throwable t) {
+ LOG.info("Async loop Exception Stacktrace is: {} ", getStackTrace(t));
--- End diff --
Why isn't the stack trace available from the log in line 372?
---