[GitHub] geode issue #296: GEODE-2109 : Calling submit on ExecutionService can cause ...
Github user deepakddixit commented on the issue: https://github.com/apache/geode/pull/296 Thanks @upthewaterspout for your assistance in resolving it. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #296: GEODE-2109 : Calling submit on ExecutionService can cause ...
Github user upthewaterspout commented on the issue: https://github.com/apache/geode/pull/296 @deepakddixit - I think using LoggingThreadGroup is definitely a better fix. The changes look good to me now, I'll merge them. Thanks! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #296: GEODE-2109 : Calling submit on ExecutionService can cause ...
Github user deepakddixit commented on the issue: https://github.com/apache/geode/pull/296 @upthewaterspout Thanks for the detailed review. I agree with you, SingleHopClientExecutor was not having uncaught exception handler which is causing exceptions not handled and ultimately not logged. Setting an uncaughtExceptionHandler would help in logging uncaught exceptions. Only change I would like to suggest is instead of adding uncaughtExceptionHandler can we set LoggingThreadGroup? The other pooled executors are using LoggingThreadGroup which handles uncauhtExceptions. I have changed code a bit and added LoggingThreadGroup, unit tests are passing. Let me know your view on using LoggingThreadGroup. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #296: GEODE-2109 : Calling submit on ExecutionService can cause ...
Github user upthewaterspout commented on the issue: https://github.com/apache/geode/pull/296 Hi @deepakddixit Sorry for the slow response. I'm a little concerned that with your changes to the test in ef4d38d, the test is no longer actually testing what happens if the runnable passed to the executor throws an exception. I think those changes *do* show that the problem is not with SystemErrRule, since with your change the test passes consistently. What that means is that there might be a product issue. I did a little bit of investigation, and I found that if I set an uncaught exception handler in the product, your original test passes consistently. I'm not quite sure why the default uncaught exception handler was not consistently working, but I think it's better to send the error to the log anyway. I stuck my changes up as a pull request to your branch if want to look at them. Let me know what you think, if this looks good to you I can merge this stuff: https://github.com/deepakddixit/incubator-geode/pull/1 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #296: GEODE-2109 : Calling submit on ExecutionService can cause ...
Github user deepakddixit commented on the issue: https://github.com/apache/geode/pull/296 @upthewaterspout Do changes look good? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #296: GEODE-2109 : Calling submit on ExecutionService can cause ...
Github user deepakddixit commented on the issue: https://github.com/apache/geode/pull/296 Precheckin and ./gradlew test passed on my machine. @upthewaterspout Kindly have a look over updated PR. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #296: GEODE-2109 : Calling submit on ExecutionService can cause ...
Github user upthewaterspout commented on the issue: https://github.com/apache/geode/pull/296 @deepakddixit I was running ./gradlew test when it failed. What's weird to me is that we're not seeing the stack trace from the test for what actually failed the test, but I think there's definitely something doing wrong here. Maybe it is a bug in SystemErrRule. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #296: GEODE-2109 : Calling submit on ExecutionService can cause ...
Github user deepakddixit commented on the issue: https://github.com/apache/geode/pull/296 @upthewaterspout It looks like from IDE when we run all tests under UnitTest category above test fails. But when we run either :geode-core:test target or complete precheckin I never observed such failure. I have observed that in IDE, SystemErrRule.getLog is not populated from thrown exception. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #296: GEODE-2109 : Calling submit on ExecutionService can cause ...
Github user upthewaterspout commented on the issue: https://github.com/apache/geode/pull/296 I looking at pulling this, and I see what one of the unit tests failed for me. It's failing intermittently in the IDE: SingleHopClientExecutorSubmitTaskWithExceptionTest.submittedTaskShouldLogFailure java.lang.RuntimeException: I am expecting this to be logged at org.apache.geode.cache.client.internal.SingleHopClientExecutorSubmitTaskWithExceptionTest$1.run(SingleHopClientExecutorSubmitTaskWithExceptionTest.java:52) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #296: GEODE-2109 : Calling submit on ExecutionService can cause ...
Github user upthewaterspout commented on the issue: https://github.com/apache/geode/pull/296 +1 looks good to me. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] geode issue #296: GEODE-2109 : Calling submit on ExecutionService can cause ...
Github user kirklund commented on the issue: https://github.com/apache/geode/pull/296 I think this is ready to commit. I'll check with @upthewaterspout. Thanks! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
