[GitHub] storm issue #2745: STORM-3135: Allow JCQueueTest to retry interrupting the c...

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


---