[
https://issues.apache.org/jira/browse/ARTEMIS-5573?focusedWorklogId=1013851&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1013851
]
ASF GitHub Bot logged work on ARTEMIS-5573:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 07/Apr/26 19:43
Start Date: 07/Apr/26 19:43
Worklog Time Spent: 10m
Work Description: tabish121 commented on code in PR #6323:
URL: https://github.com/apache/artemis/pull/6323#discussion_r3047207395
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java:
##########
@@ -198,17 +207,17 @@ private static void checkCode(int code) {
protected int messageAnnotationsPosition = VALUE_NOT_PRESENT;
protected int propertiesPosition = VALUE_NOT_PRESENT;
protected int applicationPropertiesPosition = VALUE_NOT_PRESENT;
+ protected int applicationPropertiesCount = 0;
Review Comment:
Zero is the default value for member ints so you don't need to explicitly
initialize
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java:
##########
@@ -759,6 +743,44 @@ protected synchronized void scanMessageData(ReadableBuffer
data) {
this.messageDataScanned = MessageDataScanningStatus.SCANNED.code;
}
+ public int getApplicationPropertiesCount() {
+ ensureScanning();
+ return applicationPropertiesCount;
+ }
+
+
+ // this is "borrowed" from:
+ //
https://github.com/apache/qpid-proton-j/blob/6dc5587f1d1b23969a8994f1755198e638e92bc4/proton-j/src/main/java/org/apache/qpid/proton/codec/messaging/FastPathApplicationPropertiesType.java#L93-L115
Review Comment:
This was inspired by the proton-j version but isn't a copy and it omits some
of the actual validation checks that are done in the proton-j version which are
not as thorough as they likely should be but are still better than not
validating the encoding at all.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/AMQPDecodeApplicationPropertiesTest.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.tests.integration.amqp;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.activemq.artemis.api.core.ActiveMQBuffer;
+import org.apache.activemq.artemis.api.core.ActiveMQBuffers;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.artemis.protocol.amqp.broker.AMQPMessagePersisterV2;
+import org.apache.activemq.artemis.protocol.amqp.broker.AMQPStandardMessage;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+public class AMQPDecodeApplicationPropertiesTest {
Review Comment:
Seems like a good place to add a test to validate that an AMQP message with
no application properties actually has the correct values for the new size and
count value tracking.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java:
##########
@@ -237,79 +238,10 @@ public void reloadPersistence(ActiveMQBuffer record,
CoreMessageObjectPools pool
// Message state is now that the underlying buffer is loaded, but the
contents not yet scanned
resetMessageData();
- recoverHeaderDataFromEncoding();
+ scanMessageData(data);
Review Comment:
So this changes a lot in terms of the overhead of reloading from persistence
as the scan method will decode most everything except now its been changed to
just load the bits of application properties needed to get the count and size
values. I believe that we changed how this reload from persistence worked due
to the overhead of doing this sort of full scan on reload that lead to issues
of long start times and large resource consumption from this scanning. We may
want to revert to the quick scan variation via some form of this
'recoverHeaderDataFromEncoding' type method that simply collects the bare
minimum to start and allow for a natural full scan at a later time when we
actually need that data vs the cost of this full scan on start.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java:
##########
@@ -191,20 +187,25 @@ protected ReadableBuffer getData() {
@Override
public synchronized int getMemoryEstimate() {
- if (memoryEstimate == -1) {
- if (isPaged) {
- // When the message is paged, we don't take the unmarshalled
application properties because it could be
- // updated at different places. We just keep the estimate simple
when paging.
- memoryEstimate = memoryOffset + (data != null ? data.capacity() :
0);
- } else {
- memoryEstimate = memoryOffset + (data != null ? data.capacity() +
unmarshalledApplicationPropertiesMemoryEstimateFromData(data) : 0);
- }
- originalEstimate = memoryEstimate;
+ if (memoryEstimate == VALUE_NOT_PRESENT) {
+ // This estimation was tested and validated through AMQPGlobalMaxTest
on soak-tests
+ memoryEstimate = MINIMUM_ESTIMATE + (data != null ? data.capacity() +
getApplicationPropertiesEncodingSize(data) * 2 +
getApplicationPropertiesCount() * DataConstants.SIZE_INT : 0);
}
return memoryEstimate;
}
+ private int getApplicationPropertiesEncodingSize(ReadableBuffer data) {
+ ensureScanning();
+
+ // ensureScanning will make sure we know the positions, and we will
return the encoding used based on those positions
Review Comment:
What if there is no application properties? This doesn't seem to account for
that case and would return something other than zero in that case which seems
incorrect.
Issue Time Tracking
-------------------
Worklog Id: (was: 1013851)
Time Spent: 2h 40m (was: 2.5h)
> Make AMQP Size immutable
> ------------------------
>
> Key: ARTEMIS-5573
> URL: https://issues.apache.org/jira/browse/ARTEMIS-5573
> Project: Artemis
> Issue Type: Improvement
> Components: AMQP
> Affects Versions: 2.41.0
> Reporter: Clebert Suconic
> Assignee: Clebert Suconic
> Priority: Major
> Labels: pull-request-available
> Time Spent: 2h 40m
> Remaining Estimate: 0h
>
> I have had a lot of issues, even recently on races between re-evaluating a
> message size in AMQP.
> Say a lazy decode happens at the wrong time and the memory estimates can be
> wrong.
> We have fixed issues along the years, but this is still a fragile process
> that is bound to fail. If an user for instance add a plugin breaking the
> chain of events.
> For that reason the memory estimate should already include enough estimation
> for any properties decoded and the process should be simplified.
> Less moving parts would mean less possibilities for bugs.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
