[GitHub] activemq-artemis pull request #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to ...

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] activemq-artemis pull request #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to ...

asfgit
GitHub user michaelandrepearce opened a pull request:

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

    ARTEMIS-2142 Support JMSXGroupSeq -1to close/reset group

    Add test cases
    Add GroupSequence to Message Interface
    Implement Support closing/reset group in queue impl
    Change/Fix OpenWireMessageConverter to use default of 0 if not set, for OpenWire as per documentation http://activemq.apache.org/activemq-message-properties.html

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

    $ git pull https://github.com/michaelandrepearce/activemq-artemis ARTEMIS-2142

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

    https://github.com/apache/activemq-artemis/pull/2387.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 #2387
   
----
commit 75df0dec3e4b02e4a90cdcceaf553c511ba2a483
Author: Michael André Pearce <michael.andre.pearce@...>
Date:   2018-10-22T15:31:00Z

    ARTEMIS-2142 Support JMSXGroupSeq -1to close/reset group
   
    Add test cases
    Add GroupSequence to Message Interface
    Implement Support closing/reset group in queue impl
    Change/Fix OpenWireMessageConverter to use default of 0 if not set, for OpenWire as per documentation http://activemq.apache.org/activemq-message-properties.html

----


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

[GitHub] activemq-artemis issue #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to close/r...

asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2387
 
    This is to provide feature parity with ActiveMQ 5.x, with being able to close a group using -1 group sequence.
   
     http://activemq.apache.org/message-groups.html 


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

[GitHub] activemq-artemis issue #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to close/r...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2387
 
    @clebertsuconic any objection to have this small bit in HF branch? Is causing some issues not being able to close groups


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

[GitHub] activemq-artemis issue #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to close/r...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2387
 
    I assume there isnt...ill merge tomorrow and cherry pick also.


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

[GitHub] activemq-artemis pull request #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to ...

asfgit
In reply to this post by asfgit
Github user asfgit closed the pull request at:

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


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

[GitHub] activemq-artemis issue #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to close/r...

asfgit
In reply to this post by asfgit
Github user gemmellr commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2387
 
    There are integration test failures occurring on master recently due to some issue or change with JMSXGroupSeq handling, looks to be for Openwire-->AMQP tests:
   
    > JMSLVQTest.testLVQOpenWireProducerAMQPConsumer
    JMSSelectorTest.testJMSSelectorsOpenWireProducerAMQPConsumer
     JMSWebSocketConnectionTest.testSendLargeMessageToClientFromOpenWire
   
    It may just be a coincidence that this recent change touched that area, I haven't checked further to see.


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

[GitHub] activemq-artemis issue #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to close/r...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2387
 
     Gemmellr please re check if this is actually related to this change. These passed when manually run on the branch


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

[GitHub] activemq-artemis issue #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to close/r...

asfgit
In reply to this post by asfgit
Github user gemmellr commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2387
 
    The 3 tests that failed earlier (in multiple envs) pass with this reverted, so it does seem related beyond the original coincidence of changing the same bits.


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

[GitHub] activemq-artemis issue #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to close/r...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on the issue:

    https://github.com/apache/activemq-artemis/pull/2387
 
    @gemmellr ok debugging through to see what it could be.


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

[GitHub] activemq-artemis pull request #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to ...

asfgit
In reply to this post by asfgit
Github user jdanekrh commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2387#discussion_r231901536
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
    @@ -289,6 +289,12 @@ public SimpleString getGroupID() {
           return this.getSimpleStringProperty(Message.HDR_GROUP_ID);
        }
     
    +   @Override
    +   public int getGroupSequence() {
    +      final Integer integer = this.getIntProperty(Message.HDR_GROUP_SEQUENCE);
    --- End diff --
   
    This just blew up for me. It is not Mike's bug per se, it just possibly sends execution to this little function which cannot handle `null`s
   
    https://github.com/apache/activemq-artemis/blob/10ecb358cb587c8417deeb799723d8274b0a44a5/artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java#L211-L214
   
    In my JVM,
   
    ```
        public static Integer valueOf(String var0) throws NumberFormatException {
            return parseInt(var0, 10);
        }
    ```
   
    and
   
    ```
        public static int parseInt(String var0, int var1) throws NumberFormatException {
            if (var0 == null) {
                throw new NumberFormatException("null");
    ```
   
    Here is a failing test (from cli-java project) on travis ci, https://travis-ci.org/rh-messaging/cli-java/jobs/451880325 at around
   
    > 13:28:41,911 ERROR Error while sending a message!
    > java.lang.NumberFormatException: null
   
    It was not supposed to fetch a 2.7.0 snapshot of artemis-cli-java, but somehow it did... mystery for the next time, for me or @michalxo.


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

[GitHub] activemq-artemis pull request #2387: ARTEMIS-2142 Support JMSXGroupSeq -1to ...

asfgit
In reply to this post by asfgit
Github user michaelandrepearce commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2387#discussion_r231956436
 
    --- Diff: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/message/impl/CoreMessage.java ---
    @@ -289,6 +289,12 @@ public SimpleString getGroupID() {
           return this.getSimpleStringProperty(Message.HDR_GROUP_ID);
        }
     
    +   @Override
    +   public int getGroupSequence() {
    +      final Integer integer = this.getIntProperty(Message.HDR_GROUP_SEQUENCE);
    --- End diff --
   
    The regression introduced new, is fixed by https://github.com/apache/activemq-artemis/pull/2418
   
    So the new regressions will go if/when that pr is merged


---