activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jbertram <...@git.apache.org>
Subject [GitHub] activemq-artemis pull request #2187: ARTEMIS-1545 Support JMS 2.0 Completion...
Date Thu, 02 Aug 2018 13:08:30 GMT
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2187#discussion_r207218441
  
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java
---
    @@ -189,16 +185,24 @@ public void kill() {
           this.killed = true;
        }
     
    +   private void setHandlers() {
    +      sessionChannel.setCommandConfirmationHandler(commandConfirmationHandler);
    --- End diff --
    
    I realize that your original intent was that either the confirmation or the response handler
would be set here rather than both, and this is definitely the reason for the duplicate confirmation.
 However, I couldn't determine any other way to maintain compatibility with existing bridge
behavior without setting both.  If this method is changed back to your original semantic (i.e.
only set one) then lots of tests in org.apache.activemq.artemis.tests.integration.cluster.bridge.BridgeTest
fail.  org.apache.activemq.artemis.tests.integration.client.SendAckFailTest was broken at
some point as well.  I will certainly admit that my solution here isn't the most elegant,
but I haven't found a more clever way of dealing with the failing tests.  I think eventually
the whole confirmation implementation should be refactored to deal with these issues, but
I don't see how that's feasible in a minor release.


---

Mime
View raw message