ARTEMIS-5573 and ARTEMIS-5975 Improve AMQP Size estimation#6323
ARTEMIS-5573 and ARTEMIS-5975 Improve AMQP Size estimation#6323clebertsuconic wants to merge 2 commits intoapache:mainfrom
Conversation
f70cda5 to
be90ac4
Compare
0611d5a to
03b7176
Compare
94efe35 to
70beb3a
Compare
tabish121
left a comment
There was a problem hiding this comment.
Overall looks good, nice simplification of some of the message handling
...mqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
Outdated
Show resolved
Hide resolved
...mqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
Outdated
Show resolved
Hide resolved
...mqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
Show resolved
Hide resolved
8f1e497 to
6606896
Compare
...mqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
Outdated
Show resolved
Hide resolved
...mqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
Show resolved
Hide resolved
...ocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java
Outdated
Show resolved
Hide resolved
...ocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java
Outdated
Show resolved
Hide resolved
.../org/apache/activemq/artemis/tests/integration/amqp/AMQPDecodeApplicationPropertiesTest.java
Outdated
Show resolved
Hide resolved
|
I added a commit to be squashed, returning the RELOAD state and persisting the memory estimate on the storage (journal or paging). I honestly don't think the scan will make any difference.. The real issue is that the estimates were wrong in the first place... But this commit should settle any discussion. I think There are already version tests in the Version tests. but i will make sure about it soon. |
|
for some reason I'm having to use a different persister, and I would have to digg on previous versions to understand why. I will do some investigation tomorrow, but most likely I will need the V4 persister. |
09bb86b to
540ad20
Compare
|
The reason I had to add a V4 is because of Paging: On this part here, I encode the number of queues and the queueIDs: At the point of the encoder I don't have any reference to the number of bytes used by its own decoder. I'm adding one on V4 now, if in the future anything else is added, we will stop reading at that marker. |
1cb4403 to
fa1128e
Compare
...mqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
Outdated
Show resolved
Hide resolved
|
I intend to squash the second commit on the first as soon some review is done on the new persister. |
|
I'm setting it as ready to review. but this commit should be squashed before merged. |
|
I needed to add versioning to AMQPLargeMessagePersister as well. I could choose to skip memory calculations on large message... however I will prefer to keep it the same way as StandardMessage. this is also better for future additions |
...l/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessagePersisterV4.java
Outdated
Show resolved
Hide resolved
...l/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessagePersisterV4.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessagePersisterV2.java
Outdated
Show resolved
Hide resolved
...mqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
Outdated
Show resolved
Hide resolved
...ocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java
Outdated
Show resolved
Hide resolved
- making it immutable to avoid races after updates like we had in the past - avoiding scanning after reloading by persisting extra data on storage
.../main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPLargeMessagePersisterV2.java
Show resolved
Hide resolved
...mqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
Show resolved
Hide resolved
...mqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessage.java
Outdated
Show resolved
Hide resolved
...l/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessagePersisterV4.java
Outdated
Show resolved
Hide resolved
...ocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPStandardMessage.java
Outdated
Show resolved
Hide resolved
|
I added a commit addressing the comments, let me know when I can squash it please |
No description provided.