You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "artyom-tarasenko (via GitHub)" <gi...@apache.org> on 2023/06/29 15:05:05 UTC
[GitHub] [activemq-artemis] artyom-tarasenko opened a new pull request, #4529: ARTEMIS-4095: fix delivering message size accounting
artyom-tarasenko opened a new pull request, #4529:
URL: https://github.com/apache/activemq-artemis/pull/4529
https://issues.apache.org/jira/browse/ARTEMIS-4095
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4529: ARTEMIS-4095: fix delivering message size accounting
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4529:
URL: https://github.com/apache/activemq-artemis/pull/4529#issuecomment-1613423910
Checkstyle issues:
```
Error: src/test/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumerTest.java:[52,5] (indentation) Indentation: 'member def modifier' has incorrect indentation level 4, expected level should be 3.
Error: src/test/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumerTest.java:[53,5] (indentation) Indentation: 'member def modifier' has incorrect indentation level 4, expected level should be 3.
Error: src/test/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumerTest.java:[55,5] (indentation) Indentation: 'method def modifier' has incorrect indentation level 4, expected level should be 3.
Error: src/test/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumerTest.java:[66] (regexp) RegexpSingleline: Line has trailing spaces.
Error: src/test/java/org/apache/activemq/artemis/core/protocol/openwire/amq/AMQConsumerTest.java:[67,5] (indentation) Indentation: 'method def rcurly' has incorrect indentation level 4, expected level should be 3.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] artyom-tarasenko commented on pull request #4529: ARTEMIS-4095: fix delivering message size accounting
Posted by "artyom-tarasenko (via GitHub)" <gi...@apache.org>.
artyom-tarasenko commented on PR #4529:
URL: https://github.com/apache/activemq-artemis/pull/4529#issuecomment-1613500182
> I'm still confused because the `internalSession` from `org.apache.activemq.artemis.core.protocol.openwire.OpenWireConnection` is a `org.apache.activemq.artemis.core.server.ServerSession` whereas the `session` variable being checked in `org.apache.activemq.artemis.core.protocol.openwire.amq.AMQConsumer#handleDeliver` (which you changed) is a `org.apache.activemq.artemis.core.protocol.openwire.amq.AMQSession`. I don't see the overlap here. This value appears to be coming from the OpenWire _client_.
The bug is triggered when the message is delivered to a client. The way from the client to server accounts clientid, but when the message is consumed by ra client the CONNECTION_ID_PROPERTY is removed. If I understand the flow correctly, the server opens a transaction, creates an internal session and uses it to transfer the data to a subscriber.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] artyom-tarasenko commented on pull request #4529: ARTEMIS-4095: fix delivering message size accounting
Posted by "artyom-tarasenko (via GitHub)" <gi...@apache.org>.
artyom-tarasenko commented on PR #4529:
URL: https://github.com/apache/activemq-artemis/pull/4529#issuecomment-1613435520
> This fix looks good as far as I can see, but I still don't understand why your session is being considered "internal" in the first place. Can you shed any light on this?
Basically I think it's explained in the comment here:
https://github.com/apache/activemq-artemis/blob/main/artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java#L189
InternalSessions are used for XA-Operations, so the sessions from a resource-adapter ra are internal:
https://github.com/apache/activemq-artemis/blob/main/artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java#L1464
https://github.com/apache/activemq-artemis/blob/main/artemis-protocols/artemis-openwire-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireConnection.java#L1471
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4529: ARTEMIS-4095: fix delivering message size accounting
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4529:
URL: https://github.com/apache/activemq-artemis/pull/4529#issuecomment-1615110201
> If I understand the flow correctly, the server creates an internal session, opens a transaction and uses it to transfer the data to a subscriber.
As noted previously, the `internalSession` you pointed out in `o.a.a.a.c.p.o.OpenWireConnection` is an `o.a.a.a.c.s.ServerSession`. This is a _core_ session, and it can't be used to transfer data to an OpenWire client. Furthermore, it doesn't have anything to do with the `o.a.a.a.c.p.o.a.AMQSession` for which `isInternal()` is returning `true` and triggering this problem.
I looked through the source code of the OpenWire JMS and I see a handful of places where the session ID is set to `-1` which would cause `o.a.a.a.c.p.o.a.AMQSession#isInternal()` to return `true`. I still don't understand _why_ this is done, but it helps to understand where it's coming from.
Please take care of the checkstyle issues with your PR and then I'll merge it. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram merged pull request #4529: ARTEMIS-4095: fix delivering message size accounting
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram merged PR #4529:
URL: https://github.com/apache/activemq-artemis/pull/4529
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4529: ARTEMIS-4095: fix delivering message size accounting
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4529:
URL: https://github.com/apache/activemq-artemis/pull/4529#issuecomment-1613389449
This fix looks good as far as I can see, but I still don't understand why your session is being considered "internal" in the first place. Can you shed any light on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] artyom-tarasenko commented on pull request #4529: ARTEMIS-4095: fix delivering message size accounting
Posted by "artyom-tarasenko (via GitHub)" <gi...@apache.org>.
artyom-tarasenko commented on PR #4529:
URL: https://github.com/apache/activemq-artemis/pull/4529#issuecomment-1621630755
Squashed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4529: ARTEMIS-4095: fix delivering message size accounting
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4529:
URL: https://github.com/apache/activemq-artemis/pull/4529#issuecomment-1622196981
@artyom-tarasenko, thanks for the contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4529: ARTEMIS-4095: fix delivering message size accounting
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4529:
URL: https://github.com/apache/activemq-artemis/pull/4529#issuecomment-1618197095
Please squash your commits and ensure the commit message follows the [Hacking Guide](https://github.com/apache/activemq-artemis/blob/main/docs/hacking-guide/en/code.md#commitMessageDetails). Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #4529: ARTEMIS-4095: fix delivering message size accounting
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #4529:
URL: https://github.com/apache/activemq-artemis/pull/4529#issuecomment-1613451595
I'm still confused because the `internalSession` from `org.apache.activemq.artemis.core.protocol.openwire.OpenWireConnection` is a `org.apache.activemq.artemis.core.server.ServerSession` whereas the `session` variable being checked in `org.apache.activemq.artemis.core.protocol.openwire.amq.AMQConsumer#handleDeliver` (which you changed) is a `org.apache.activemq.artemis.core.protocol.openwire.amq.AMQSession`. I don't see the overlap here. This value appears to be coming from the OpenWire _client_.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org