You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by srdo <gi...@git.apache.org> on 2018/06/29 20:44:27 UTC
[GitHub] storm pull request #2745: STORM-3135: Allow JCQueueTest to retry interruptin...
GitHub user srdo opened a pull request:
https://github.com/apache/storm/pull/2745
STORM-3135: Allow JCQueueTest to retry interrupting the consumer if t…
…he queue happens to be full
https://issues.apache.org/jira/browse/STORM-3135
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/srdo/storm STORM-3135
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2745.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 #2745
----
commit 938829d1a242cb4a1368d98755598a869b276e4c
Author: Stig Rohde Døssing <sr...@...>
Date: 2018-06-29T20:42:36Z
STORM-3135: Allow JCQueueTest to retry interrupting the consumer if the queue happens to be full
----
---
[GitHub] storm issue #2745: STORM-3135: Allow JCQueueTest to retry interrupting the c...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2745
I have two concerns about this fix.
1. We are doing something in the test that we are not doing anywhere in the real code. If this is a bug in the test why are we not doing something similar in the real code that tries to shut down the queue, and if we want to do something similar everywhere, can we make some changes to `JCQueue` so the halt is guaranteed to happen.
2. The second one goes along with this. Part of `haltWithInterrupt` is best effort (inserting the `INTERRUPT` message), but the rest of the method assumes it will be called once (closes down metrics etc.) . Not sure if these need to be moved to a different location when we know the close has happened, or if we want to protect them from being executed multiple times. But all of this depends on how we address my first concern.
---
[GitHub] storm issue #2745: STORM-3135: Allow JCQueueTest to retry interrupting the c...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2745
Yes, the thread is still interrupted after the exception is thrown. I'll change it to use `interrupted` instead.
---
[GitHub] storm issue #2745: STORM-3135: Allow JCQueueTest to retry interrupting the c...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2745
Sorry I don't think I submitted my nit. We are calling `isInterrupted` in asyncLoop. Is this going to leave the state of the thread as interrupted after we throw the exception.
---
[GitHub] storm issue #2745: STORM-3135: Allow JCQueueTest to retry interrupting the c...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2745
The failure is unrelated, raised an issue at https://issues.apache.org/jira/browse/STORM-3137 to track.
---
[GitHub] storm pull request #2745: STORM-3135: Allow JCQueueTest to retry interruptin...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2745
---
[GitHub] storm issue #2745: STORM-3135: Allow JCQueueTest to retry interrupting the c...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2745
@revans2 Good point. It probably doesn't hurt much in the real code, because the queue is only shut down when the worker or executor is shut down. It could be an issue for tests with LocalCluster though.
Took a different approach. As far as I can tell, there isn't a great reason that we need to interrupt the async loops through the JCQueue. We can just interrupt the threads directly, which is already happening during worker/executor shutdown.
I think the reason to interrupt via the queue is because the wait strategies now include use of LockSupport.parkNanos, so there's no guarantee that the executors will hit a Thread.sleep and throw an InterruptedException. We can fix this by checking manually for interrupts, either in the async loop, or in the wait strategies where we're calling parkNanos.
---
[GitHub] storm issue #2745: STORM-3135: Allow JCQueueTest to retry interrupting the c...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2745
Fixed.
---