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

2017-10-19 Thread mtaylor
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 issue #1590: ARTEMIS-1464 Fix Core to AMQP conversion Bytes...

2017-10-19 Thread michaelandrepearce
Github user michaelandrepearce commented on the issue: https://github.com/apache/activemq-artemis/pull/1590 @mtaylor do we have your +1? ---

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

2017-10-19 Thread gemmellr
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

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

2017-10-19 Thread michaelandrepearce
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...

2017-10-17 Thread michaelandrepearce
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...

2017-10-17 Thread mtaylor
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:

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

2017-10-17 Thread michaelandrepearce
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...

2017-10-17 Thread gemmellr
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...

2017-10-17 Thread michaelandrepearce
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,

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

2017-10-17 Thread gemmellr
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

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

2017-10-17 Thread michaelandrepearce
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...

2017-10-17 Thread gemmellr
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

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

2017-10-17 Thread michaelandrepearce
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

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

2017-10-17 Thread mtaylor
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)