You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2020/03/16 11:53:09 UTC
[GitHub] [activemq-artemis] franz1981 opened a new pull request #3019:
ARTEMIS-2658 AMQP message read from page has wrong encode size
franz1981 opened a new pull request #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #3019:
ARTEMIS-2658 AMQP message read from page has wrong encode size
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019#issuecomment-599664538
@clebertsuconic I'm not very proud of https://github.com/apache/activemq-artemis/pull/3019/commits/2105479304a7efead14ecb343686bf31042f1731#diff-d3a97ffe6cb7d4ab3fd0c4d24dea022bR100, but it seems covered by some existing tests eg `AmqpBridgeClusterRedistributionTest`. This change should preserve the nice performance of https://issues.apache.org/jira/browse/ARTEMIS-2617: I'm now running a test to verify it.
@tabish121 is aware of this issue ie duplicate decoding of `address` : I'm thinking to create a `AMQPMessagePersisterV3` that save the encoding/decoding of `address` to happen twice, while "fixing" `AMQPMessagePersisterV2` trying to preserve the nice optimization on journal loading. wdyt? advices?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #3019:
ARTEMIS-2658 AMQP message read from page has wrong encode size
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019#issuecomment-599664538
@clebertsuconic I'm not very proud of https://github.com/apache/activemq-artemis/pull/3019/commits/2105479304a7efead14ecb343686bf31042f1731#diff-d3a97ffe6cb7d4ab3fd0c4d24dea022bR100, but it seems covered by some existing tests eg `AmqpBridgeClusterRedistributionTest`. This change should preserve the nice performance optimization of https://issues.apache.org/jira/browse/ARTEMIS-2617: I'm now running a test to verify it.
@tabish121 is aware of this issue ie duplicate decoding of `address` : I'm thinking to create a `AMQPMessagePersisterV3` that save the encoding/decoding of `address` to happen twice, while "fixing" `AMQPMessagePersisterV2` trying to preserve the nice optimization on journal loading. wdyt? advices?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] asfgit closed pull request #3019: ARTEMIS-2658
AMQP message read from page has wrong encode size
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019#discussion_r393231077
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessagePersisterV2.java
##########
@@ -72,25 +75,36 @@ public void encode(ActiveMQBuffer buffer, Message record) {
}
@Override
- public Message decode(ActiveMQBuffer buffer, Message record, CoreMessageObjectPools pool) {
- AMQPMessage message = (AMQPMessage) super.decode(buffer, record, pool);
+ public Message decode(ActiveMQBuffer buffer, Message ignore, CoreMessageObjectPools pool) {
+ // IMPORTANT:
+ // This is a sightly modified copy of the AMQPMessagePersister::decode body
+ // to save extraProperties to be created twice: this would kill GC during journal loading
+ long id = buffer.readLong();
+ long format = buffer.readLong();
+ // this instance is being used only if there are no extraProperties or just for debugging purposes:
+ // on journal loading pool shouldn't be null so it shouldn't create any garbage.
+ final SimpleString address;
+ if (pool == null) {
+ address = buffer.readNullableSimpleString();
+ } else {
+ address = SimpleString.readNullableSimpleString(buffer.byteBuf(), pool.getAddressDecoderPool());
+ }
+ AMQPStandardMessage record = new AMQPStandardMessage(format);
+ record.reloadPersistence(buffer, pool);
+ record.setMessageID(id);
+ // END of AMQPMessagePersister::decode body copy
int size = buffer.readInt();
-
if (size != 0) {
- // message::setAddress could have populated extra properties
- // hence, we can safely replace the value on the properties
- // if it has been encoded differently in the rest of the buffer
- TypedProperties existingExtraProperties = message.getExtraProperties();
- TypedProperties extraProperties = existingExtraProperties;
- if (existingExtraProperties == null) {
- extraProperties = new TypedProperties(Message.INTERNAL_PROPERTY_NAMES_PREDICATE);
- }
- extraProperties.decode(buffer.byteBuf(), pool != null ? pool.getPropertiesDecoderPools() : null, existingExtraProperties == null);
- if (extraProperties != existingExtraProperties) {
- message.setExtraProperties(extraProperties);
- }
+ final TypedProperties extraProperties = record.createExtraProperties();
+ extraProperties.decode(buffer.byteBuf(), pool != null ? pool.getPropertiesDecoderPools() : null);
+ assert Objects.equals(address, extraProperties.getSimpleStringProperty(AMQPMessage.ADDRESS_PROPERTY)) :
Review comment:
Can't we just log.debug if something is wrong here?
log.warn...
i would rather have a big warning on the system, with the system somewhat working, than having AssertException and having the system freezing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #3019:
ARTEMIS-2658 AMQP message read from page has wrong encode size
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019#issuecomment-599664538
@clebertsuconic I'm not very proud of https://github.com/apache/activemq-artemis/pull/3019/commits/2105479304a7efead14ecb343686bf31042f1731#diff-d3a97ffe6cb7d4ab3fd0c4d24dea022bR100, but it seems covered by some existing tests eg `AmqpBridgeClusterRedistributionTest`. This change should preserve the nice performance optimization of https://issues.apache.org/jira/browse/ARTEMIS-2617: I'm now running a test to verify it.
@tabish121 is aware of this issue ie duplicate decoding of `address` : I'm thinking to create a `AMQPMessagePersisterV3` that save the encoding/decoding of `address` to happen twice. wdyt? advices?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3019: ARTEMIS-2658
AMQP message read from page has wrong encode size
Posted by GitBox <gi...@apache.org>.
franz1981 commented on issue #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019#issuecomment-599679621
@clebertsuconic The issue is more evident on master, so this PR should be fine...I'll open a JIRA to investigate the performance issue while loading the journal separately...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3019: ARTEMIS-2658
AMQP message read from page has wrong encode size
Posted by GitBox <gi...@apache.org>.
franz1981 commented on issue #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019#issuecomment-599664538
@clebertsuconic I'm not very proud of https://github.com/apache/activemq-artemis/pull/3019/commits/2105479304a7efead14ecb343686bf31042f1731#diff-d3a97ffe6cb7d4ab3fd0c4d24dea022bR100, but it seems covered by some existing tests eg `AmqpBridgeClusterRedistributionTest`. It should retain the nice performance of https://issues.apache.org/jira/browse/ARTEMIS-2617: I'm now running a test to verify it.
@tabish121 is aware of this issue ie duplicate decoding of `address` : I'm thinking to create a `AMQPMessagePersisterV3` that save the encoding/decoding of `address` to happen twice, while "fixing" `AMQPMessagePersisterV2` trying to preserve the nice optimization on journal loading. wdyt? advices?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3019: ARTEMIS-2658
AMQP message read from page has wrong encode size
Posted by GitBox <gi...@apache.org>.
franz1981 commented on issue #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019#issuecomment-599669786
@clebertsuconic Seems that regardless my efforts of trying to preserve the perf optimization...this PR still have some perf issue see
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019#discussion_r393231077
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessagePersisterV2.java
##########
@@ -72,25 +75,36 @@ public void encode(ActiveMQBuffer buffer, Message record) {
}
@Override
- public Message decode(ActiveMQBuffer buffer, Message record, CoreMessageObjectPools pool) {
- AMQPMessage message = (AMQPMessage) super.decode(buffer, record, pool);
+ public Message decode(ActiveMQBuffer buffer, Message ignore, CoreMessageObjectPools pool) {
+ // IMPORTANT:
+ // This is a sightly modified copy of the AMQPMessagePersister::decode body
+ // to save extraProperties to be created twice: this would kill GC during journal loading
+ long id = buffer.readLong();
+ long format = buffer.readLong();
+ // this instance is being used only if there are no extraProperties or just for debugging purposes:
+ // on journal loading pool shouldn't be null so it shouldn't create any garbage.
+ final SimpleString address;
+ if (pool == null) {
+ address = buffer.readNullableSimpleString();
+ } else {
+ address = SimpleString.readNullableSimpleString(buffer.byteBuf(), pool.getAddressDecoderPool());
+ }
+ AMQPStandardMessage record = new AMQPStandardMessage(format);
+ record.reloadPersistence(buffer, pool);
+ record.setMessageID(id);
+ // END of AMQPMessagePersister::decode body copy
int size = buffer.readInt();
-
if (size != 0) {
- // message::setAddress could have populated extra properties
- // hence, we can safely replace the value on the properties
- // if it has been encoded differently in the rest of the buffer
- TypedProperties existingExtraProperties = message.getExtraProperties();
- TypedProperties extraProperties = existingExtraProperties;
- if (existingExtraProperties == null) {
- extraProperties = new TypedProperties(Message.INTERNAL_PROPERTY_NAMES_PREDICATE);
- }
- extraProperties.decode(buffer.byteBuf(), pool != null ? pool.getPropertiesDecoderPools() : null, existingExtraProperties == null);
- if (extraProperties != existingExtraProperties) {
- message.setExtraProperties(extraProperties);
- }
+ final TypedProperties extraProperties = record.createExtraProperties();
+ extraProperties.decode(buffer.byteBuf(), pool != null ? pool.getPropertiesDecoderPools() : null);
+ assert Objects.equals(address, extraProperties.getSimpleStringProperty(AMQPMessage.ADDRESS_PROPERTY)) :
Review comment:
Can't we just log.debug if something is wrong here?
log.warn...
i would rather have a big warning on the system, with the system somewhat working, than having AssertException and having the system freezing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #3019:
ARTEMIS-2658 AMQP message read from page has wrong encode size
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019#issuecomment-599679621
@clebertsuconic The perf issue is still present and more evident on master, so this PR should be fine...I'll open a JIRA to investigate the performance issue while loading the journal separately...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] asfgit merged pull request #3019: ARTEMIS-2658
AMQP message read from page has wrong encode size
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #3019: ARTEMIS-2658 AMQP message read from page has wrong encode size
URL: https://github.com/apache/activemq-artemis/pull/3019
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services