New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARTEMIS-1498: Openwire internal headers should not be part of message #1793

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@RaiSaurabh
Contributor

RaiSaurabh commented Jan 19, 2018

No description provided.

@franz1981

This comment has been minimized.

Show comment
Hide comment
@franz1981

franz1981 Jan 19, 2018

Contributor

Please check this too #1786
Probably will be easier to wait mine to be merged: the changes I've pushed are including some refactoring that will make this change to be easier, wdyt?

Contributor

franz1981 commented Jan 19, 2018

Please check this too #1786
Probably will be easier to wait mine to be merged: the changes I've pushed are including some refactoring that will make this change to be easier, wdyt?

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh

RaiSaurabh Jan 19, 2018

Contributor

Ok, @franz1981 I will wait for the merging.

Contributor

RaiSaurabh commented Jan 19, 2018

Ok, @franz1981 I will wait for the merging.

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic Jan 22, 2018

Contributor

Let me see if I understand.. you implemented OpenwireMessage here?

Contributor

clebertsuconic commented Jan 22, 2018

Let me see if I understand.. you implemented OpenwireMessage here?

@franz1981

This comment has been minimized.

Show comment
Hide comment
@franz1981

franz1981 Jan 22, 2018

Contributor

Please take a look here:

2018-01-22 17:28:59,899 WARN  [org.apache.activemq.artemis.core.server] Error during message dispatch: java.lang.RuntimeException: null
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireMessage.toCore(OpenwireMessage.java:787) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireMessage.toCore(OpenwireMessage.java:779) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenWireMessageConverter.toAMQMessage(OpenWireMessageConverter.java:454) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenWireMessageConverter.createMessageDispatch(OpenWireMessageConverter.java:435) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.amq.AMQConsumer.handleDeliver(AMQConsumer.java:222) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.amq.AMQSession.sendMessage(AMQSession.java:276) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.server.impl.ServerConsumerImpl.deliverStandardMessage(ServerConsumerImpl.java:1091) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.server.impl.ServerConsumerImpl.proceedDeliver(ServerConsumerImpl.java:460) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.server.impl.QueueImpl.proceedDeliver(QueueImpl.java:2763) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.server.impl.QueueImpl.deliver(QueueImpl.java:2247) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.server.impl.QueueImpl.access$1900(QueueImpl.java:106) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.server.impl.QueueImpl$DeliverRunner.run(QueueImpl.java:3021) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.utils.actors.OrderedExecutor.doTask(OrderedExecutor.java:42) [artemis-commons-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.utils.actors.OrderedExecutor.doTask(OrderedExecutor.java:31) [artemis-commons-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.utils.actors.ProcessorBase.executePendingTasks(ProcessorBase.java:66) [artemis-commons-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [rt.jar:1.8.0_102]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [rt.jar:1.8.0_102]
	at java.lang.Thread.run(Thread.java:745) [rt.jar:1.8.0_102]
Caused by: java.lang.NumberFormatException: null
	at java.lang.Integer.parseInt(Integer.java:542) [rt.jar:1.8.0_102]
	at java.lang.Integer.valueOf(Integer.java:766) [rt.jar:1.8.0_102]
	at org.apache.activemq.artemis.utils.collections.TypedProperties.getIntProperty(TypedProperties.java:225) [artemis-commons-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireMessage.getIntProperty(OpenwireMessage.java:681) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireMessage.getIntProperty(OpenwireMessage.java:600) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireCoreConverter.toCore(OpenwireCoreConverter.java:74) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireConverter.toCore(OpenwireConverter.java:42) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireMessage.toCore(OpenwireMessage.java:785) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	... 17 more

I've built up a 1 producer 1 consumer test vs 1 not durable JMS queue and I'm getting it.
The idea seems good as @clebertsuconic as shown me but need to address these issues first 👍

Contributor

franz1981 commented Jan 22, 2018

Please take a look here:

2018-01-22 17:28:59,899 WARN  [org.apache.activemq.artemis.core.server] Error during message dispatch: java.lang.RuntimeException: null
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireMessage.toCore(OpenwireMessage.java:787) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireMessage.toCore(OpenwireMessage.java:779) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenWireMessageConverter.toAMQMessage(OpenWireMessageConverter.java:454) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenWireMessageConverter.createMessageDispatch(OpenWireMessageConverter.java:435) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.amq.AMQConsumer.handleDeliver(AMQConsumer.java:222) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.amq.AMQSession.sendMessage(AMQSession.java:276) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.server.impl.ServerConsumerImpl.deliverStandardMessage(ServerConsumerImpl.java:1091) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.server.impl.ServerConsumerImpl.proceedDeliver(ServerConsumerImpl.java:460) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.server.impl.QueueImpl.proceedDeliver(QueueImpl.java:2763) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.server.impl.QueueImpl.deliver(QueueImpl.java:2247) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.server.impl.QueueImpl.access$1900(QueueImpl.java:106) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.server.impl.QueueImpl$DeliverRunner.run(QueueImpl.java:3021) [artemis-server-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.utils.actors.OrderedExecutor.doTask(OrderedExecutor.java:42) [artemis-commons-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.utils.actors.OrderedExecutor.doTask(OrderedExecutor.java:31) [artemis-commons-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.utils.actors.ProcessorBase.executePendingTasks(ProcessorBase.java:66) [artemis-commons-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [rt.jar:1.8.0_102]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [rt.jar:1.8.0_102]
	at java.lang.Thread.run(Thread.java:745) [rt.jar:1.8.0_102]
Caused by: java.lang.NumberFormatException: null
	at java.lang.Integer.parseInt(Integer.java:542) [rt.jar:1.8.0_102]
	at java.lang.Integer.valueOf(Integer.java:766) [rt.jar:1.8.0_102]
	at org.apache.activemq.artemis.utils.collections.TypedProperties.getIntProperty(TypedProperties.java:225) [artemis-commons-2.5.0-SNAPSHOT.jar:2.5.0-SNAPSHOT]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireMessage.getIntProperty(OpenwireMessage.java:681) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireMessage.getIntProperty(OpenwireMessage.java:600) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireCoreConverter.toCore(OpenwireCoreConverter.java:74) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireConverter.toCore(OpenwireConverter.java:42) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	at org.apache.activemq.artemis.core.protocol.openwire.OpenwireMessage.toCore(OpenwireMessage.java:785) [artemis-openwire-protocol-2.5.0-SNAPSHOT.jar:]
	... 17 more

I've built up a 1 producer 1 consumer test vs 1 not durable JMS queue and I'm getting it.
The idea seems good as @clebertsuconic as shown me but need to address these issues first 👍

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce Jan 22, 2018

Contributor

Really good work, I like this, really good to see OpenWire getting this :) As noted already seems some corner cases have some odd behaviour / errors.

Have you run the full OpenWire test suite? I know not all run in the PR build, so worth checking. Might be worth running the full suite even if you can.

Contributor

michaelandrepearce commented Jan 22, 2018

Really good work, I like this, really good to see OpenWire getting this :) As noted already seems some corner cases have some odd behaviour / errors.

Have you run the full OpenWire test suite? I know not all run in the PR build, so worth checking. Might be worth running the full suite even if you can.

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic Jan 22, 2018

Contributor

I ran the whole testsuite and it didn't complete.

it's a very nice start though.

Contributor

clebertsuconic commented Jan 22, 2018

I ran the whole testsuite and it didn't complete.

it's a very nice start though.

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh

RaiSaurabh Feb 1, 2018

Contributor

Thanks, @michaelandrepearce @clebertsuconic and @franz1981 for the review comments. I have tried to fix it. Could you please review the changes now.

Contributor

RaiSaurabh commented Feb 1, 2018

Thanks, @michaelandrepearce @clebertsuconic and @franz1981 for the review comments. I have tried to fix it. Could you please review the changes now.

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh

RaiSaurabh Feb 1, 2018

Contributor

@michaelandrepearce updated the code as per your comments.

Contributor

RaiSaurabh commented Feb 1, 2018

@michaelandrepearce updated the code as per your comments.

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh

RaiSaurabh Feb 1, 2018

Contributor

@michaelandrepearce Seems there is some problem with the Git due to which this build failed. Could you please check and let me know.

Contributor

RaiSaurabh commented Feb 1, 2018

@michaelandrepearce Seems there is some problem with the Git due to which this build failed. Could you please check and let me know.

@franz1981

This comment has been minimized.

Show comment
Hide comment
@franz1981

franz1981 Feb 1, 2018

Contributor

@RaiSaurabh Please rebase it and force push it to re-trigger a Jenkins build and hopefully it will pass

Contributor

franz1981 commented Feb 1, 2018

@RaiSaurabh Please rebase it and force push it to re-trigger a Jenkins build and hopefully it will pass

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce Feb 1, 2018

Contributor

@franz1981 I think there’s a general Jenkins PR build issue. If you have direct ping ability or nudge directly Martyn or Clebert or someone on PM ?

Contributor

michaelandrepearce commented Feb 1, 2018

@franz1981 I think there’s a general Jenkins PR build issue. If you have direct ping ability or nudge directly Martyn or Clebert or someone on PM ?

@franz1981

This comment has been minimized.

Show comment
Hide comment
@franz1981

franz1981 Feb 1, 2018

Contributor

@michaelandrepearce I will warn the PM squad 👍

Contributor

franz1981 commented Feb 1, 2018

@michaelandrepearce I will warn the PM squad 👍

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh

RaiSaurabh Feb 2, 2018

Contributor

Thanks, @michaelandrepearce @franz1981 for the review. I have implemented the changes.

Contributor

RaiSaurabh commented Feb 2, 2018

Thanks, @michaelandrepearce @franz1981 for the review. I have implemented the changes.

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh

RaiSaurabh Feb 14, 2018

Contributor

@michaelandrepearce Looks like another issue with GIT repo. Could you please check.

Contributor

RaiSaurabh commented Feb 14, 2018

@michaelandrepearce Looks like another issue with GIT repo. Could you please check.

@franz1981

This comment has been minimized.

Show comment
Hide comment
@franz1981

franz1981 Feb 14, 2018

Contributor

@RaiSaurabh Please check the @michaelandrepearce comments about this PR first 👍

Contributor

franz1981 commented Feb 14, 2018

@RaiSaurabh Please check the @michaelandrepearce comments about this PR first 👍

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh

RaiSaurabh Feb 14, 2018

Contributor

@franz1981 I have implemented the changes @michaelandrepearce requested. @michaelandrepearce could you please review it.

Contributor

RaiSaurabh commented Feb 14, 2018

@franz1981 I have implemented the changes @michaelandrepearce requested. @michaelandrepearce could you please review it.

@franz1981

This comment has been minimized.

Show comment
Hide comment
@franz1981

franz1981 Feb 14, 2018

Contributor

@RaiSaurabh I'm doing some preliminary benchmarks to see if the number of buffer copies are decreased 👍

Contributor

franz1981 commented Feb 14, 2018

@RaiSaurabh I'm doing some preliminary benchmarks to see if the number of buffer copies are decreased 👍

@franz1981

This comment has been minimized.

Show comment
Hide comment
@franz1981

franz1981 Feb 14, 2018

Contributor

@RaiSaurabh @michaelandrepearce
I've run a couple of benchmarks and the results are strange, perf wire the improvements are on par or (slightly) less than #1849, but the number are "strange" eg:

4 producers - 4 consumers
------------------------------------
EndToEnd Throughput: 145985 ops/sec
EndToEnd SERVICE-TIME Latencies distribution in MICROSECONDS
mean           10032775.17
min             9999220.74
50.00%         10066329.60
90.00%         10066329.60
99.00%         10066329.60
99.90%         10066329.60
99.99%         10066329.60
max            10066329.60
count               400000
---------------------------------
1 producer - 1 consumer
---------------------------------
EndToEnd Throughput: 78247 ops/sec
EndToEnd SERVICE-TIME Latencies distribution in MICROSECONDS
mean           10032775.17
min             9999220.74
50.00%         10066329.60
90.00%         10066329.60
99.00%         10066329.60
99.90%         10066329.60
99.99%         10066329.60
max            10066329.60
count               100000

It is weird: the end-2-end latencies are stucked being ALWAYS the same with performances a little less from the PR above: probably there is something not working on message translation, please takes a look to the Open Wire tests to spot if everything is ok...

Contributor

franz1981 commented Feb 14, 2018

@RaiSaurabh @michaelandrepearce
I've run a couple of benchmarks and the results are strange, perf wire the improvements are on par or (slightly) less than #1849, but the number are "strange" eg:

4 producers - 4 consumers
------------------------------------
EndToEnd Throughput: 145985 ops/sec
EndToEnd SERVICE-TIME Latencies distribution in MICROSECONDS
mean           10032775.17
min             9999220.74
50.00%         10066329.60
90.00%         10066329.60
99.00%         10066329.60
99.90%         10066329.60
99.99%         10066329.60
max            10066329.60
count               400000
---------------------------------
1 producer - 1 consumer
---------------------------------
EndToEnd Throughput: 78247 ops/sec
EndToEnd SERVICE-TIME Latencies distribution in MICROSECONDS
mean           10032775.17
min             9999220.74
50.00%         10066329.60
90.00%         10066329.60
99.00%         10066329.60
99.90%         10066329.60
99.99%         10066329.60
max            10066329.60
count               100000

It is weird: the end-2-end latencies are stucked being ALWAYS the same with performances a little less from the PR above: probably there is something not working on message translation, please takes a look to the Open Wire tests to spot if everything is ok...

@Override
public void receiveBuffer(ByteBuf buffer) {

This comment has been minimized.

@michaelandrepearce

michaelandrepearce Feb 14, 2018

Contributor

This should be implemented

@michaelandrepearce

michaelandrepearce Feb 14, 2018

Contributor

This should be implemented

}
@Override
public void sendBuffer(ByteBuf buffer, int deliveryCount) {

This comment has been minimized.

@michaelandrepearce

michaelandrepearce Feb 14, 2018

Contributor

Needs implementing?

@michaelandrepearce

michaelandrepearce Feb 14, 2018

Contributor

Needs implementing?

Show outdated Hide outdated .../org/apache/activemq/artemis/core/protocol/openwire/OpenWireMessage.java
@Override
public void messageChanged() {
}

This comment has been minimized.

@michaelandrepearce

michaelandrepearce Feb 14, 2018

Contributor

Does this need implementing

@michaelandrepearce

michaelandrepearce Feb 14, 2018

Contributor

Does this need implementing

This comment has been minimized.

@RaiSaurabh

RaiSaurabh Feb 16, 2018

Contributor

No not at the moment

@RaiSaurabh

RaiSaurabh Feb 16, 2018

Contributor

No not at the moment

Show outdated Hide outdated .../org/apache/activemq/artemis/core/protocol/openwire/OpenWireMessage.java
Show outdated Hide outdated .../org/apache/activemq/artemis/core/protocol/openwire/OpenWireMessage.java
Show outdated Hide outdated .../org/apache/activemq/artemis/core/protocol/openwire/OpenWireMessage.java
Show outdated Hide outdated .../org/apache/activemq/artemis/core/protocol/openwire/OpenWireMessage.java
Show outdated Hide outdated .../org/apache/activemq/artemis/core/protocol/openwire/OpenWireMessage.java
@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce Feb 14, 2018

Contributor

Really need PR build to be green. Maybe re-push with a comment update to get try trigger another build.

Seems some methods have been still left in-implemented. Also could/should look to use string to SimpleString pools.

Lastly have you checked that this persists to disk and is recoverable without converting to core.

Contributor

michaelandrepearce commented Feb 14, 2018

Really need PR build to be green. Maybe re-push with a comment update to get try trigger another build.

Seems some methods have been still left in-implemented. Also could/should look to use string to SimpleString pools.

Lastly have you checked that this persists to disk and is recoverable without converting to core.

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic Feb 14, 2018

Contributor

for this kind of big PR to be merged, we need the full testsuite to be passing.

Not talking about the PR testsuite.

There's an issue that we don't have an env at apache where we can run the whole tests.. so we have to do it on our boxes. I have a box at home where I do this. .and another box at work.

basically:

mvn -Ptests test

and the same on the openwire tests..

Contributor

clebertsuconic commented Feb 14, 2018

for this kind of big PR to be merged, we need the full testsuite to be passing.

Not talking about the PR testsuite.

There's an issue that we don't have an env at apache where we can run the whole tests.. so we have to do it on our boxes. I have a box at home where I do this. .and another box at work.

basically:

mvn -Ptests test

and the same on the openwire tests..

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh

RaiSaurabh Feb 16, 2018

Contributor

Seems some issue with Travis and Jenkins for this project is disabled.

Contributor

RaiSaurabh commented Feb 16, 2018

Seems some issue with Travis and Jenkins for this project is disabled.

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh

RaiSaurabh Feb 16, 2018

Contributor

@michaelandrepearce I have implemented the changes asked and also checked that this persists to disk and is recoverable on restart of the broker without converting to the core message.
Could you please review it.

Contributor

RaiSaurabh commented Feb 16, 2018

@michaelandrepearce I have implemented the changes asked and also checked that this persists to disk and is recoverable on restart of the broker without converting to the core message.
Could you please review it.

@franz1981

This comment has been minimized.

Show comment
Hide comment
@franz1981

franz1981 Feb 16, 2018

Contributor

@RaiSaurabh I've just checked vs the integration/openwire tests in integration-tests and it fail 22 tests/338.
The original number of failing tests here is 1, so anything over this value is actually a regression.
I'm not runining the compatibility ones on activemq5-unit-tests (my notebook could die before finishing them :P) with no reasons.
Please run the tests I've mentioned and try at least to no have regressions there: the idea of the original PR is good (very good), but the implementation need to be fixed in order to not have huge regressions on how OpenWire is working, thanks 👍

Contributor

franz1981 commented Feb 16, 2018

@RaiSaurabh I've just checked vs the integration/openwire tests in integration-tests and it fail 22 tests/338.
The original number of failing tests here is 1, so anything over this value is actually a regression.
I'm not runining the compatibility ones on activemq5-unit-tests (my notebook could die before finishing them :P) with no reasons.
Please run the tests I've mentioned and try at least to no have regressions there: the idea of the original PR is good (very good), but the implementation need to be fixed in order to not have huge regressions on how OpenWire is working, thanks 👍

@jbertram

This comment has been minimized.

Show comment
Hide comment
@jbertram

jbertram Feb 16, 2018

Contributor

@RaiSaurabh, the Travis CI issues should be resolved. Can you please rebase this PR and push -f? Thanks!

Contributor

jbertram commented Feb 16, 2018

@RaiSaurabh, the Travis CI issues should be resolved. Can you please rebase this PR and push -f? Thanks!

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh

RaiSaurabh Mar 16, 2018

Contributor

@michaelandrepearce @clebertsuconic Apologies for delaying this and dragging this long. I tried to fix the code to pass all the test cases of OpenWire but still, I get 5 failed test cases. I would require your contribution/guidance in fixing the same.

Following are the failed test cases:
OpenWireGroupingTest.testGrouping
JmsTopicRequestReplyTest.testSendAndReceive
RedeliveryPolicyTest.testRedeliveryPolicyPerDestination
TemporaryQueueClusterTest.testClusteredQueue

Contributor

RaiSaurabh commented Mar 16, 2018

@michaelandrepearce @clebertsuconic Apologies for delaying this and dragging this long. I tried to fix the code to pass all the test cases of OpenWire but still, I get 5 failed test cases. I would require your contribution/guidance in fixing the same.

Following are the failed test cases:
OpenWireGroupingTest.testGrouping
JmsTopicRequestReplyTest.testSendAndReceive
RedeliveryPolicyTest.testRedeliveryPolicyPerDestination
TemporaryQueueClusterTest.testClusteredQueue

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce Mar 16, 2018

Contributor

@RaiSaurabh great stuff, if I get a chance I’ll try have a look. If not hopefully Clebert will be able to.

Contributor

michaelandrepearce commented Mar 16, 2018

@RaiSaurabh great stuff, if I get a chance I’ll try have a look. If not hopefully Clebert will be able to.

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce Mar 26, 2018

Contributor

@RaiSaurabh im struggling to rebase this on top of master. Did you rebase after @franz1981's work?

Contributor

michaelandrepearce commented Mar 26, 2018

@RaiSaurabh im struggling to rebase this on top of master. Did you rebase after @franz1981's work?

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh

RaiSaurabh Mar 26, 2018

Contributor

@michaelandrepearce Yes, I did.

Contributor

RaiSaurabh commented Mar 26, 2018

@michaelandrepearce Yes, I did.

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce Mar 26, 2018

Contributor

@RaiSaurabh could you rebase your branch on current master then, for me?
e.g. bits introduced by @franz1981

coreMessage.putStringProperty(AMQ_MSG_GROUP_ID, coreMessageObjectPools.getGroupIdStringSimpleStringPool().getOrCreate(groupId));

are not present in yours, and thus the conflicts.

Contributor

michaelandrepearce commented Mar 26, 2018

@RaiSaurabh could you rebase your branch on current master then, for me?
e.g. bits introduced by @franz1981

coreMessage.putStringProperty(AMQ_MSG_GROUP_ID, coreMessageObjectPools.getGroupIdStringSimpleStringPool().getOrCreate(groupId));

are not present in yours, and thus the conflicts.

@RaiSaurabh

This comment has been minimized.

Show comment
Hide comment
@RaiSaurabh
Contributor

RaiSaurabh commented Mar 26, 2018

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce Mar 26, 2018

Contributor

@RaiSaurabh thanks, thats making my life easier, just looking at this atm, if you hadn't guessed :)

Contributor

michaelandrepearce commented Mar 26, 2018

@RaiSaurabh thanks, thats making my life easier, just looking at this atm, if you hadn't guessed :)

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce Mar 26, 2018

Contributor

@RaiSaurabh have sent a PR to your branch, that solves some bits for you.

Contributor

michaelandrepearce commented Mar 26, 2018

@RaiSaurabh have sent a PR to your branch, that solves some bits for you.

michaelandrepearce and others added some commits Mar 26, 2018

ARTEMIS-1498 - Openwire Refactor - fix message groups
Expose setGroupID as top level Message method, as getGroupId already is, and update all.

Expose setGroupSequence and getGroupSequence as a top level Messsage method as is JMS spec defined (behaviour is not) 

Use object pools in open wire message

Update activemq5 unit tests to use artemis-junit (seems class was moved at somepoint)
@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic Mar 27, 2018

Contributor

There should be some simplification on the converters here. They shouldn't be needed at all when talking openwire <-> openwire. I saw some issues when I was looking into this.

Contributor

clebertsuconic commented Mar 27, 2018

There should be some simplification on the converters here. They shouldn't be needed at all when talking openwire <-> openwire. I saw some issues when I was looking into this.

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce Apr 18, 2018

Contributor

@clebertsuconic you able to give @RaiSaurabh a pr with what you mean. I’ve gone though it myself and in general looks fine from what I can tell, eg it has a check to see if it’s of openwire message type and if it is it returns it instead of converting

Contributor

michaelandrepearce commented Apr 18, 2018

@clebertsuconic you able to give @RaiSaurabh a pr with what you mean. I’ve gone though it myself and in general looks fine from what I can tell, eg it has a check to see if it’s of openwire message type and if it is it returns it instead of converting

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic Apr 18, 2018

Contributor

this is a nice start.. but it will require some work... a lot of tests are broken... the converters need to be updated... (especially on the openwire testsuite).

Contributor

clebertsuconic commented Apr 18, 2018

this is a nice start.. but it will require some work... a lot of tests are broken... the converters need to be updated... (especially on the openwire testsuite).

@michaelandrepearce

This comment has been minimized.

Show comment
Hide comment
@michaelandrepearce

michaelandrepearce Apr 18, 2018

Contributor

i Got a lot of test failures previous, but it seemed a lot better when I ran a suite a few weeks back, is that since his latest changes and work that you ran it? I’m just conscious this would be a really big +100 on the whole ActiveMQ 6 thing that we all desperately want. Eg that’s why I’m starting to look at the page of bits and trying to implement slowly, even so personally we don’t use openwire clients

Contributor

michaelandrepearce commented Apr 18, 2018

i Got a lot of test failures previous, but it seemed a lot better when I ran a suite a few weeks back, is that since his latest changes and work that you ran it? I’m just conscious this would be a really big +100 on the whole ActiveMQ 6 thing that we all desperately want. Eg that’s why I’m starting to look at the page of bits and trying to implement slowly, even so personally we don’t use openwire clients

@clebertsuconic

This comment has been minimized.

Show comment
Hide comment
@clebertsuconic

clebertsuconic Apr 18, 2018

Contributor

@michaelandrepearce There is another testsuite just for openwire. somethings that we inherited from activemq5.. it's under ./tests/activemq5-unit-tests

that one was fairly broken. i would need to re-check it.

Contributor

clebertsuconic commented Apr 18, 2018

@michaelandrepearce There is another testsuite just for openwire. somethings that we inherited from activemq5.. it's under ./tests/activemq5-unit-tests

that one was fairly broken. i would need to re-check it.

@franz1981

This comment has been minimized.

Show comment
Hide comment
@franz1981

franz1981 Apr 22, 2018

Contributor

@clebertsuconic @RaiSaurabh @michaelandrepearce
Re performances
I've the first results using just a small benchmark with 1 producer/1 consumer using 100 bytes messages.

this PR:    37050 msg/sec
2.5:        85689 msg/sec

So performance-wise there is a long way to go, but I think it is related to some correctness issues.

Re correctness
I've launched a run on CI with the OpenWire test suite with no success:

[INFO] ActiveMQ5.x unit tests ............................. FAILURE [  2.950 s]

So I've decided to run some on my box@home.
The results from org.apache.activemq.artemis.tests.integration.openwire ones shows 7 failed tests/341:

CompressedInteropTest.testCoreReceiveOpenWireCompressedMessages
MessageListenerRedeliveryTest.testQueueSessionListenerExceptionDlq
OpenWireLargeMessageTest.testSendReceiveLargeMessage
GeneralInteropTest.testSendingToCoreJms
SimpleOpenWireTest.testTempQueueSendAfterConnectionClose
RedeliveryPolicyTest.testRedeliveryPolicyPerDestination
RedeliveryPolicyTest.testDLQHandling

I've to run the tests/activemq5-unit-tests yet but I suppose that's enough to be fixed.
Sorry for letting this PR sitting here for such a long time and thanks @RaiSaurabh to have worked on it: I think that given the right direction taken by the work on this PR makes a lot of sense to fix all the compatibility issues first, before looking to the performance gains that in the current state, are not present. @RaiSaurabh don't give up with the hard work done here and take a look at the tests results: as much as possible I will try to help by reviewing the code and propose fixes 👍

Contributor

franz1981 commented Apr 22, 2018

@clebertsuconic @RaiSaurabh @michaelandrepearce
Re performances
I've the first results using just a small benchmark with 1 producer/1 consumer using 100 bytes messages.

this PR:    37050 msg/sec
2.5:        85689 msg/sec

So performance-wise there is a long way to go, but I think it is related to some correctness issues.

Re correctness
I've launched a run on CI with the OpenWire test suite with no success:

[INFO] ActiveMQ5.x unit tests ............................. FAILURE [  2.950 s]

So I've decided to run some on my box@home.
The results from org.apache.activemq.artemis.tests.integration.openwire ones shows 7 failed tests/341:

CompressedInteropTest.testCoreReceiveOpenWireCompressedMessages
MessageListenerRedeliveryTest.testQueueSessionListenerExceptionDlq
OpenWireLargeMessageTest.testSendReceiveLargeMessage
GeneralInteropTest.testSendingToCoreJms
SimpleOpenWireTest.testTempQueueSendAfterConnectionClose
RedeliveryPolicyTest.testRedeliveryPolicyPerDestination
RedeliveryPolicyTest.testDLQHandling

I've to run the tests/activemq5-unit-tests yet but I suppose that's enough to be fixed.
Sorry for letting this PR sitting here for such a long time and thanks @RaiSaurabh to have worked on it: I think that given the right direction taken by the work on this PR makes a lot of sense to fix all the compatibility issues first, before looking to the performance gains that in the current state, are not present. @RaiSaurabh don't give up with the hard work done here and take a look at the tests results: as much as possible I will try to help by reviewing the code and propose fixes 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment