[GitHub] activemq-artemis pull request #1590: ARTEMIS-1464 Fix Core to AMQP conversio...

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

[GitHub] activemq-artemis pull request #1590: ARTEMIS-1464 Fix Core to AMQP conversio...

pgfox
GitHub user michaelandrepearce opened a pull request:

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

    ARTEMIS-1464 Fix Core to AMQP conversion BytesMessage corrupts bytes

    Extend test cases in MessageTypesTest to cover Core to AMQP combinations for all JMSTypeTests
    Fix ServerJMSBytesMessage to correctly return bodyLength
    Fix CoreAmqpConverter to use amqp type "Data" instead of "AmqpValue" to match the same as when BytesMessage sent by Qpid Jms
    Remove unused/dead code

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

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

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

    https://github.com/apache/activemq-artemis/pull/1590.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 #1590
   
----
commit ae7842df61bd189cf6dbeaa727cdc34f43b0a107
Author: Michael André Pearce <[hidden email]>
Date:   2017-10-17T08:20:34Z

    ARTEMIS-1464 Fix Core to AMQP conversion BytesMessage corrupts bytes
   
    Extend test cases in MessageTypesTest to cover Core to AMQP combinations for all JMSTypeTests
    Fix ServerJMSBytesMessage to correctly return bodyLength
    Fix CoreAmqpConverter to use amqp type "Data" instead of "AmqpValue" to match the same as when BytesMessage sent by Qpid Jms
    Remove unused/dead code

----


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

pgfox
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1590
 
    @michaelandrepearce Hey, looks like there are two test failures, could you take a look.
   
    ```
    Test Result (2 failures / +2)
    org.apache.activemq.artemis.protocol.amqp.converter.message.JMSMappingOutboundTransformerTest.testConvertEmptyBytesMessageToAmqpMessageWithAmqpValueBody
    org.apache.activemq.artemis.protocol.amqp.converter.message.JMSMappingOutboundTransformerTest.testConvertUncompressedBytesMessageToAmqpMessageWithAmqpValueBody
    ```



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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

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

    https://github.com/apache/activemq-artemis/pull/1590
 
    @mtaylor so this test is interesting, essentially it is stating that for BytesMessage it should be 'AmqpValue' type, where as on testing with Qpid JMS Producer a BytesMessage body is 'Data' type.
   
    I assume Qpid behaviour is more correct, but i want to get a second opinion before i adjust this test to reflect it.


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

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

    https://github.com/apache/activemq-artemis/pull/1590
 
    Using an AmqpValue section with Binary content is perfectly valid, and is what some other clients can prefer to do by default. The use of AmqpValue vs Data section shouldn't influence here, just what is prepared and put in them, so I wouldn't change it until there was reason.


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

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

    https://github.com/apache/activemq-artemis/pull/1590
 
    @gemmellr fair enough this explains difference with QPID.
   
    re Oasis spec doc i found can you comment? It seems to be specific on the type.


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

[GitHub] activemq-artemis pull request #1590: ARTEMIS-1464 Fix Core to AMQP conversio...

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

    https://github.com/apache/activemq-artemis/pull/1590#discussion_r145071633
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/JMSMessageTypesTest.java ---
    @@ -158,11 +162,29 @@ public void testBytesMessageSendReceive() throws Throwable {
        }
     
        @Test(timeout = 60000)
    -   public void testMessageSendReceive() throws Throwable {
    +   public void testBytesMessageSendReceiveFromAMQPToAMQP() throws Throwable {
    +      testBytesMessageSendReceive(createConnection(), createConnection());
    +   }
    +
    +   @Test(timeout = 60000)
    +   public void testBytesMessageSendReceiveFromCoreToAMQP() throws Throwable {
    +      testBytesMessageSendReceive(createCoreConnection(), createConnection());
    +   }
    +
    +   @Test(timeout = 60000)
    +   public void testBytesMessageSendReceiveFromCoreToCore() throws Throwable {
    --- End diff --
   
    This (and the others like it) seems like it should be a duplicate of an existing test. Even if not, having it under the amqp test package doesn't seem that great.


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

[GitHub] activemq-artemis pull request #1590: ARTEMIS-1464 Fix Core to AMQP conversio...

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

    https://github.com/apache/activemq-artemis/pull/1590#discussion_r145072050
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/JMSMessageTypesTest.java ---
    @@ -158,11 +162,29 @@ public void testBytesMessageSendReceive() throws Throwable {
        }
     
        @Test(timeout = 60000)
    -   public void testMessageSendReceive() throws Throwable {
    +   public void testBytesMessageSendReceiveFromAMQPToAMQP() throws Throwable {
    +      testBytesMessageSendReceive(createConnection(), createConnection());
    +   }
    +
    +   @Test(timeout = 60000)
    +   public void testBytesMessageSendReceiveFromCoreToAMQP() throws Throwable {
    +      testBytesMessageSendReceive(createCoreConnection(), createConnection());
    +   }
    +
    +   @Test(timeout = 60000)
    +   public void testBytesMessageSendReceiveFromCoreToCore() throws Throwable {
    --- End diff --
   
    Its so we check cross compatibly to and from amqp. see other similar test cases, the core to core ones are just completeness on the combinations.


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

[GitHub] activemq-artemis pull request #1590: ARTEMIS-1464 Fix Core to AMQP conversio...

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

    https://github.com/apache/activemq-artemis/pull/1590#discussion_r145081692
 
    --- Diff: tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/JMSMessageTypesTest.java ---
    @@ -158,11 +162,29 @@ public void testBytesMessageSendReceive() throws Throwable {
        }
     
        @Test(timeout = 60000)
    -   public void testMessageSendReceive() throws Throwable {
    +   public void testBytesMessageSendReceiveFromAMQPToAMQP() throws Throwable {
    +      testBytesMessageSendReceive(createConnection(), createConnection());
    +   }
    +
    +   @Test(timeout = 60000)
    +   public void testBytesMessageSendReceiveFromCoreToAMQP() throws Throwable {
    +      testBytesMessageSendReceive(createCoreConnection(), createConnection());
    +   }
    +
    +   @Test(timeout = 60000)
    +   public void testBytesMessageSendReceiveFromCoreToCore() throws Throwable {
    --- End diff --
   
    I wasn't commenting about the amqp test variations, just that core to core test and the others added like it. If those aren't duplicates of existing tests I'd be surprised, and they are a waste of build time if they are. However, even if they aren't, this does not feel like the place for them, in a package directed at testing amqp support. If they fail they wont indicate any issue with the amqp support someone might reasonably think they are testing. It doesn't seem like a place where anyone would really expect to go looking for them if they did want to check for duplicates.


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

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

    https://github.com/apache/activemq-artemis/pull/1590
 
    The AMQP JMS mapping is in part around getting any different AMQP JMS implementations behaving consistently with each other, it could as easily say use amqp-value, or say use either. The JMS mapping/client also copes with receiving either, see the recieving side mapping details later in the doc.
   
    There is nothing wrong with the broker sending amqp-value sections, and other clients might prefer that. It seems entirely unrelated to the actual defect here, so I'd leave it the way it is, but even if it were changed I would actually separate it out as its own specific change.


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

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

    https://github.com/apache/activemq-artemis/pull/1590
 
    @gemmellr yeah, i reverted that specific change put back to "amqpvalue" also just removing the additional core->core test combinations leaving just, amqp->amqp, core->amqp, amqp->core combinations


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

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

    https://github.com/apache/activemq-artemis/pull/1590
 
    ..and looks like you changed it while I was commenting as such ;)


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

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

    https://github.com/apache/activemq-artemis/pull/1590
 
    @gemmellr  :)


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

pgfox
In reply to this post by pgfox
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1590
 
    Looks like you guys already have this resolved.  FYI the corresponding core to core test can be found here: https://github.com/apache/activemq-artemis/blob/master/tests/jms-tests/src/test/java/org/apache/activemq/artemis/jms/tests/message/MessageBodyTest.java#L51
   



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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

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

    https://github.com/apache/activemq-artemis/pull/1590
 
    @mtaylor @gemmellr thanks for the review comments.


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

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

    https://github.com/apache/activemq-artemis/pull/1590
 
    @mtaylor @gemmellr I'm going to merge this, this morning , as it seems no further feedback.


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

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

    https://github.com/apache/activemq-artemis/pull/1590
 
    I don't have any more comments but I was mainly reviewing from the AMQP handling perspective, I don't know enough about the Core bits usage to be comfortable about merging it myself (though obviously the tests added suggest its good) and so defer to those who do.


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

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

    https://github.com/apache/activemq-artemis/pull/1590
 
    @mtaylor do we have your +1?


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

[GitHub] activemq-artemis pull request #1590: ARTEMIS-1464 Fix Core to AMQP conversio...

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

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


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

[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

pgfox
In reply to this post by pgfox
Github user mtaylor commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1590
 
    @michaelandrepearce Yes +1 from me.  Nice test coverage.


---