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 2021/01/14 17:31:23 UTC
[GitHub] [activemq-artemis] gtully opened a new pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
gtully opened a new pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407
…eck for modification after potential filter execution
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic merged pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
Posted by GitBox <gi...@apache.org>.
clebertsuconic merged pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407#discussion_r562416623
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3716,6 +3730,16 @@ private boolean deliver(final MessageReference ref) {
}
}
+ private void accountForChangeInMemoryEstimate(final MessageReference ref, final int existingMemoryEstimate) {
+ final int delta = ref.getMessageMemoryEstimate() - existingMemoryEstimate;
+ if (delta > 0) {
+ PagingStore pageStore = ref.getOwner();
+ if (pageStore != null) {
+ pageStore.addSize(delta);
Review comment:
2 observations:
1. what happen to core messages here?
2. this method could become static
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407#discussion_r561860237
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java
##########
@@ -124,12 +124,24 @@ protected ReadableBuffer getData() {
@Override
public int getMemoryEstimate() {
if (memoryEstimate == -1) {
- memoryEstimate = memoryOffset + (data != null ? data.capacity() : 0);
+ memoryEstimate = memoryOffset + (data != null ? data.capacity() + unmarshalledApplicationPropertiesMemoryEstimateFromData() : 0);
}
return memoryEstimate;
}
+ private int unmarshalledApplicationPropertiesMemoryEstimateFromData() {
+ if (applicationProperties != null) {
+ // they have been unmarshalled, estimate memory usage based on their encoded size
+ if (remainingBodyPosition != VALUE_NOT_PRESENT) {
+ return remainingBodyPosition - applicationPropertiesPosition;
Review comment:
pushed a rebase to see if there are any more failures in the mix
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407#discussion_r557688999
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java
##########
@@ -124,12 +124,24 @@ protected ReadableBuffer getData() {
@Override
public int getMemoryEstimate() {
if (memoryEstimate == -1) {
- memoryEstimate = memoryOffset + (data != null ? data.capacity() : 0);
+ memoryEstimate = memoryOffset + (data != null ? data.capacity() + unmarshalledApplicationPropertiesMemoryEstimateFromData() : 0);
}
return memoryEstimate;
}
+ private int unmarshalledApplicationPropertiesMemoryEstimateFromData() {
+ if (applicationProperties != null) {
+ // they have been unmarshalled, estimate memory usage based on their encoded size
+ if (remainingBodyPosition != VALUE_NOT_PRESENT) {
+ return remainingBodyPosition - applicationPropertiesPosition;
Review comment:
it's important to always return the same size...
When the message enters the system, you're adding the size of the message, when leaving you're returning the size after parsing the property.
You need to cache the value, otherwise you would get inconsistencies found by the test suite here:
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendReceiveAMQP[isNetty=true, persistent=true] 5.4 sec 1
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendAMQPReceiveCore[isNetty=true, persistent=true] 5.2 sec 1
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendAMQPReceiveOpenWire[isNetty=true, persistent=true] 5.3 sec 1
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendReceiveAMQP[isNetty=true, persistent=false] 5.3 sec 1
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendAMQPReceiveCore[isNetty=true, persistent=false] 5.2 sec 1
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendAMQPReceiveOpenWire[isNetty=true, persistent=false] 5.3 sec 1
org.apache.activemq.artemis.tests.integration.cluster.failover.ReplicatedManyMultipleServerFailoverNoNodeGroupNameTest.testStartLiveFirst
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407#discussion_r561860237
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java
##########
@@ -124,12 +124,24 @@ protected ReadableBuffer getData() {
@Override
public int getMemoryEstimate() {
if (memoryEstimate == -1) {
- memoryEstimate = memoryOffset + (data != null ? data.capacity() : 0);
+ memoryEstimate = memoryOffset + (data != null ? data.capacity() + unmarshalledApplicationPropertiesMemoryEstimateFromData() : 0);
}
return memoryEstimate;
}
+ private int unmarshalledApplicationPropertiesMemoryEstimateFromData() {
+ if (applicationProperties != null) {
+ // they have been unmarshalled, estimate memory usage based on their encoded size
+ if (remainingBodyPosition != VALUE_NOT_PRESENT) {
+ return remainingBodyPosition - applicationPropertiesPosition;
Review comment:
pushed a rebase to see if there are any more failures in the mix
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407#discussion_r559754464
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java
##########
@@ -124,12 +124,24 @@ protected ReadableBuffer getData() {
@Override
public int getMemoryEstimate() {
if (memoryEstimate == -1) {
- memoryEstimate = memoryOffset + (data != null ? data.capacity() : 0);
+ memoryEstimate = memoryOffset + (data != null ? data.capacity() + unmarshalledApplicationPropertiesMemoryEstimateFromData() : 0);
}
return memoryEstimate;
}
+ private int unmarshalledApplicationPropertiesMemoryEstimateFromData() {
+ if (applicationProperties != null) {
+ // they have been unmarshalled, estimate memory usage based on their encoded size
+ if (remainingBodyPosition != VALUE_NOT_PRESENT) {
+ return remainingBodyPosition - applicationPropertiesPosition;
Review comment:
the intent is to compensate for any change in the estimate, in the knowledge that it may happen for filter evaluation or for transformation. But I must not have all the possibilities covered, and it may be tricky. There is deliver and directDeliver etc.
The consumerTest is now sorted, but I need to investigate next org.apache.activemq.artemis.tests.integration.amqp.connect.AMQPReplicaTest#testReplicaCatchupOnQueueCreates
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407#discussion_r557688999
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java
##########
@@ -124,12 +124,24 @@ protected ReadableBuffer getData() {
@Override
public int getMemoryEstimate() {
if (memoryEstimate == -1) {
- memoryEstimate = memoryOffset + (data != null ? data.capacity() : 0);
+ memoryEstimate = memoryOffset + (data != null ? data.capacity() + unmarshalledApplicationPropertiesMemoryEstimateFromData() : 0);
}
return memoryEstimate;
}
+ private int unmarshalledApplicationPropertiesMemoryEstimateFromData() {
+ if (applicationProperties != null) {
+ // they have been unmarshalled, estimate memory usage based on their encoded size
+ if (remainingBodyPosition != VALUE_NOT_PRESENT) {
+ return remainingBodyPosition - applicationPropertiesPosition;
Review comment:
it's important to always return the same size...
When the message enters the system, you're adding the size of the message, when leaving you're returning the size after parsing the property.
You need to cache the value, otherwise you would get inconsistencies found by the test suite here:
Test Name | Duration | Age
-- | -- | --
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendReceiveAMQP[isNetty=true, persistent=true] | 5.4 sec | 1
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendAMQPReceiveCore[isNetty=true, persistent=true] | 5.2 sec | 1
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendAMQPReceiveOpenWire[isNetty=true, persistent=true] | 5.3 sec | 1
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendReceiveAMQP[isNetty=true, persistent=false] | 5.3 sec | 1
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendAMQPReceiveCore[isNetty=true, persistent=false] | 5.2 sec | 1
org.apache.activemq.artemis.tests.integration.client.ConsumerTest.testSendAMQPReceiveOpenWire[isNetty=true, persistent=false] | 5.3 sec | 1
org.apache.activemq.artemis.tests.integration.cluster.failover.ReplicatedManyMultipleServerFailoverNoNodeGroupNameTest.testStartLiveFirst | 43 sec | 1
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] gtully commented on pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
Posted by GitBox <gi...@apache.org>.
gtully commented on pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407#issuecomment-760348512
This fix seems a little indirect to me, but I did not see a better way. Am open to suggestions!
----------------------------------------------------------------
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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407#discussion_r565400084
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3716,6 +3730,16 @@ private boolean deliver(final MessageReference ref) {
}
}
+ private void accountForChangeInMemoryEstimate(final MessageReference ref, final int existingMemoryEstimate) {
+ final int delta = ref.getMessageMemoryEstimate() - existingMemoryEstimate;
+ if (delta > 0) {
+ PagingStore pageStore = ref.getOwner();
+ if (pageStore != null) {
+ pageStore.addSize(delta);
Review comment:
yes to the static, let me fix that, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407#discussion_r559783349
##########
File path: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java
##########
@@ -124,12 +124,24 @@ protected ReadableBuffer getData() {
@Override
public int getMemoryEstimate() {
if (memoryEstimate == -1) {
- memoryEstimate = memoryOffset + (data != null ? data.capacity() : 0);
+ memoryEstimate = memoryOffset + (data != null ? data.capacity() + unmarshalledApplicationPropertiesMemoryEstimateFromData() : 0);
}
return memoryEstimate;
}
+ private int unmarshalledApplicationPropertiesMemoryEstimateFromData() {
+ if (applicationProperties != null) {
+ // they have been unmarshalled, estimate memory usage based on their encoded size
+ if (remainingBodyPosition != VALUE_NOT_PRESENT) {
+ return remainingBodyPosition - applicationPropertiesPosition;
Review comment:
hmm. org.apache.activemq.artemis.tests.integration.amqp.connect.AMQPReplicaTest#testReplicaCatchupOnQueueCreates looks to be timing dependent, unrelated to the changes as far as i can 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
[GitHub] [activemq-artemis] gtully commented on a change in pull request #3407: ARTEMIS-3067 - track application properties in memory estimate and ch…
Posted by GitBox <gi...@apache.org>.
gtully commented on a change in pull request #3407:
URL: https://github.com/apache/activemq-artemis/pull/3407#discussion_r565397776
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java
##########
@@ -3716,6 +3730,16 @@ private boolean deliver(final MessageReference ref) {
}
}
+ private void accountForChangeInMemoryEstimate(final MessageReference ref, final int existingMemoryEstimate) {
+ final int delta = ref.getMessageMemoryEstimate() - existingMemoryEstimate;
+ if (delta > 0) {
+ PagingStore pageStore = ref.getOwner();
+ if (pageStore != null) {
+ pageStore.addSize(delta);
Review comment:
thanks franz,
a core message won't have any delta that I am aware of, it's memory estimate does not change, but you were in that space recently, does it change?
----------------------------------------------------------------
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