You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by michaelandrepearce <gi...@git.apache.org> on 2017/10/17 08:28:53 UTC
[GitHub] activemq-artemis pull request #1590: ARTEMIS-1464 Fix Core to AMQP conversio...
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 <mi...@me.com>
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
----
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by michaelandrepearce <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by gemmellr <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1590
@gemmellr :)
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by mtaylor <gi...@git.apache.org>.
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
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by michaelandrepearce <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by gemmellr <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by gemmellr <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on the issue:
https://github.com/apache/activemq-artemis/pull/1590
@michaelandrepearce Yes +1 from me. Nice test coverage.
---
[GitHub] activemq-artemis pull request #1590: ARTEMIS-1464 Fix Core to AMQP conversio...
Posted by michaelandrepearce <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by gemmellr <gi...@git.apache.org>.
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 ;)
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by mtaylor <gi...@git.apache.org>.
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
```
---
[GitHub] activemq-artemis pull request #1590: ARTEMIS-1464 Fix Core to AMQP conversio...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1590
---
[GitHub] activemq-artemis pull request #1590: ARTEMIS-1464 Fix Core to AMQP conversio...
Posted by gemmellr <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1590
@mtaylor @gemmellr thanks for the review comments.
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by michaelandrepearce <gi...@git.apache.org>.
Github user michaelandrepearce commented on the issue:
https://github.com/apache/activemq-artemis/pull/1590
@mtaylor do we have your +1?
---
[GitHub] activemq-artemis pull request #1590: ARTEMIS-1464 Fix Core to AMQP conversio...
Posted by gemmellr <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by michaelandrepearce <gi...@git.apache.org>.
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.
---
[GitHub] activemq-artemis issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...
Posted by michaelandrepearce <gi...@git.apache.org>.
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
---