[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[kishorvpatil]

Github user kishorvpatil closed the pull request at:

https://github.com/apache/storm/pull/2638


---

[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[HeartSaVioR]

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 pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[kishorvpatil]

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 pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[srdo]

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...

[kishorvpatil]

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 pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[srdo]

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? 


---

[GitHub] storm pull request #2638: [STORM-3034] Adding exception stacktrace for execu...

[srdo]

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...

[Ethanlm]

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...

[kishorvpatil]

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 
Date:   2018-04-19T14:01:27Z

Adding exception stacktrace for executor failures in worker




---