[GitHub] activemq-artemis pull request #1761: [ARTEMIS-1527] - [Artemis Testsuite] Ac...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1761: [ARTEMIS-1527] - [Artemis Testsuite] Ac...

jgoodyear-2
GitHub user JiriOndrusek opened a pull request:

    https://github.com/apache/activemq-artemis/pull/1761

    [ARTEMIS-1527] - [Artemis Testsuite] ActiveMQMessageHandlerTest#testS…

    …erverShutdownAndReconnect fails
   
    Issue: https://issues.apache.org/jira/browse/ARTEMIS-1527

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/JiriOndrusek/activemq-artemis ARTEMIS-1527-fix

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/1761.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 #1761
   
----
commit 367e633846b020bb002e0cc0ff228df6bddefaa3
Author: JiriOndrusek <jondruse@...>
Date:   2018-01-09T08:24:03Z

    [ARTEMIS-1527] - [Artemis Testsuite] ActiveMQMessageHandlerTest#testServerShutdownAndReconnect fails

----


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1761: [ARTEMIS-1527] - [Artemis Testsuite] Ac...

jgoodyear-2
Github user JiriOndrusek commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1761#discussion_r160420636
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/RemotingConnectionImpl.java ---
    @@ -205,14 +211,59 @@ public void fail(final ActiveMQException me, String scaleDownTargetNodeID) {
           }
     
           // Then call the listeners
    -      callFailureListeners(me, scaleDownTargetNodeID);
    +      List<CountDownLatch> failureLatches = callFailureListeners(me, scaleDownTargetNodeID);
     
           callClosingListeners();
     
    -      internalClose();
    +      CountDownLatch latch = new CountDownLatch(1);
     
    -      for (Channel channel : channels.values()) {
    -         channel.returnBlocking(me);
    +      new ScheduledInternalClose(failureLatches, me, latch).run();
    +
    +      return latch;
    +   }
    +
    +   // internalClose has to be called after failureListeners are finished.
    +   private class ScheduledInternalClose implements Runnable {
    +
    +      private final List<CountDownLatch> failureLatches;
    +      private final ActiveMQException me;
    +      private final CountDownLatch latch;
    +
    +      public ScheduledInternalClose(List<CountDownLatch> failureLatches, final ActiveMQException me, CountDownLatch latch) {
    +         this.failureLatches = failureLatches;
    +         this.me = me;
    +         this.latch = latch;
    +      }
    +
    +      @Override
    +      public void run() {
    +         List<CountDownLatch> running = new LinkedList<>();
    +         failureLatches.forEach((l) -> {
    +            try {
    +               //interval is defined during scheduled execution
    +               if (!l.await(1, TimeUnit.MILLISECONDS)) {
    +                  running.add(l);
    +               }
    +            } catch (InterruptedException e) {
    +               ActiveMQClientLogger.LOGGER.warn(e.getMessage(), e);
    +            }
    +         });
    +
    +         if (!running.isEmpty()) {
    +            //TODO(jondruse) is there constant or propertyF usable for this kind of wait interval?
    +            scheduledExecutorService.schedule(new ScheduledInternalClose(running, me, latch), 500, TimeUnit.MILLISECONDS);
    --- End diff --
   
    I have too use "constant" for repetitive checking, whether action is already finished. May I use some generic timeout used in project (or define new attribute, ...)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1761: [ARTEMIS-1527] - [Artemis Testsuite] ActiveMQM...

jgoodyear-2
In reply to this post by jgoodyear-2
Github user JiriOndrusek commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1761
 
    In 2 places I have to use "constant" for repetitive checking, whether action is already finished. May I use some generic timeout used in project (or define new attribute, ...)
    (ClientSessionFactoryImpl line 1123 and RemotingConnectionImpl line 254)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #1761: [ARTEMIS-1527] - [Artemis Testsuite] Ac...

jgoodyear-2
In reply to this post by jgoodyear-2
Github user JiriOndrusek commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/1761#discussion_r160421530
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/client/impl/ClientSessionFactoryImpl.java ---
    @@ -990,17 +1111,26 @@ public CloseRunnable(RemotingConnection conn, String scaleDownTargetNodeID) {
           // can cause reconnect loop
           @Override
           public void run() {
    -         try {
    -            CLOSE_RUNNABLES.add(this);
    -            if (scaleDownTargetNodeID == null) {
    -               conn.fail(ActiveMQClientMessageBundle.BUNDLE.disconnected());
    -            } else {
    -               conn.fail(ActiveMQClientMessageBundle.BUNDLE.disconnected(), scaleDownTargetNodeID);
    -            }
    -         } finally {
    -            CLOSE_RUNNABLES.remove(this);
    +         CLOSE_RUNNABLES.add(this);
    +         CountDownLatch latch;
    +         if (scaleDownTargetNodeID == null) {
    +            latch = conn.fail(ActiveMQClientMessageBundle.BUNDLE.disconnected());
    +         } else {
    +            latch = conn.fail(ActiveMQClientMessageBundle.BUNDLE.disconnected(), scaleDownTargetNodeID);
              }
     
    +         //TODO(jondruse) is there constant or property usable for this kind of wait interval?
    --- End diff --
   
    I have too use "constant" for repetitive checking, whether action is already finished. May I use some generic timeout used in project (or define new attribute, ...)


---
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis issue #1761: [ARTEMIS-1527] - [Artemis Testsuite] ActiveMQM...

jgoodyear-2
In reply to this post by jgoodyear-2
Github user clebertsuconic commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1761
 
    -1000 on this PR...
   
    it's a brittle change over a test failure. .you could find other way to fix the test?
   
   
    it's a huge change on the maintenance 1.x branch... it's a risky change... if this is a go... it needs to be on master. (I wouldn't ever cherry-pick something this size into 1.x anyways).


---