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 into
Conversation
clebertsuconic
force-pushed
the
message-size-amqp
branch
8 times, most recently
from
f70cda5 to
be90ac4
Compare
clebertsuconic
changed the title
clebertsuconic
force-pushed
the
message-size-amqp
branch
9 times, most recently
from
0611d5a to
03b7176
Compare
clebertsuconic
force-pushed
the
message-size-amqp
branch
8 times, most recently
from
94efe35 to
70beb3a
Compare
clebertsuconic
changed the title
tabish121
left a comment
tabish121
left a comment
There was a problem hiding this comment.
Overall looks good, nice simplification of some of the message handling
clebertsuconic
force-pushed
the
message-size-amqp
branch
2 times, most recently
from
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. |
clebertsuconic
force-pushed
the
message-size-amqp
branch
4 times, most recently
from
1cb4403 to
fa1128e
Compare
|
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 |
|
I added a commit addressing the comments, let me know when I can squash it please |
|
I added another commit to be squashed after review |
|
when I worked on this I wasn't planning to rewrite persistence for AMQP messages. I will work on something for those persisters. |
|
I am now using a MacCodec instead of a streamed bytes. I am not using a Hashmap to store records in between, but there's a codec that will store it like it was a hashMap. I also added a clustering compatibility tests since the persistence is used with clustering. |
gemmellr
left a comment
gemmellr
left a comment
There was a problem hiding this comment.
Gave it a skim over time, havent closely looked at most of it, but still had various comments so here they are. Most important one being, I do not think this should be in 2.54.0 at this stage.
|
@gemmellr I am marking this as draft because of the versioning.. whenever we decide to merge it I will change it and make it ready to commit |
- making it immutable to avoid races after updates like we had in the past - avoiding scanning after reloading by persisting extra data on storage
|
I have added a commit addressing some of the comments.. I still need to finish this , but it will be tomorrow. for now I have made this a draft again. |
|
Only point I have open now is the discussion on the ThreadLocal for AMQPMessageMetadataPersister. Making it stateless will require intermediate state somewhere else... or a bigger change in the order messages are created versus reloaded. (I want to avoid such inversion at this point) |
|
@gemmellr I just pushed one commit to address the MapCodec Thread Local (well, not really you wills see) Now the codec itself is singleton and the Metadata has a thread local. I couldn't safely refactor the message to be created before the metadata itself. At least now it's clearer why I need the thread local as it's not about the codec itself, but rather about the data needing to be stored before the messages are instantiated. |
3 participants
No description provided.