ARTEMIS-5573 and ARTEMIS-5975 Improve AMQP Size estimation#6323
ARTEMIS-5573 and ARTEMIS-5975 Improve AMQP Size estimation#6323clebertsuconic wants to merge 13 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
|
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. |
|
@gemmellr actually, I wasn't even working when I realized this is a very lightweight object. the JVM should handle an allocation like this very well. Look at my last commit, I think I can just let it be a regular Object, no need for a thread local, simplifying it further. I will take a few days in vacation... so I won't answer this for a few days |
|
@gemmellr all comments were addressed.. I rebased it after the 2.55 release. Do you want another round of review on this? |
|
I do, yes. I havent had time to look again yet, or at your previous changes either, in part as I took your prior comment to mean it was getting changed again. I cant immediately tell what has changed since I last looked, the helpful comparison links appear to have vanished, so I guess I need to review the whole thing at this point. |
- making it immutable to avoid races after updates like we had in the past - avoiding scanning after reloading by persisting extra data on storage
…way with the thread local
|
@gemmellr I will place the commits back |
|
@gemmellr I have place the original commits back... although I have rebased against main now. |
|
I will squash these commits when you're done with the review... thank you |
Add checkReadableBytes() before each decode loop on AbstractMapPersister read to prevent malicious entries count from exceeding the declared record size. reverted a not needed change in large message.
…ort on number of entries. This would actually avoid negative values. And we use a short anyway as a key
|
some review of my own here; MapPersister could use unsigned short for entries. (no need to verify if negative and it saves some space.. I use short for keys.. so I can't ever have more than short on entries). It's a separate commit. |
| // Replication compatibility is tested FORWARD ONLY (older main -> newer backup). | ||
| // Artemis 2.55.0 introduced AMQPMessagePersisterV4, which breaks backward replication compatibility. | ||
| // This means 2.55.0+ main servers cannot replicate to pre-2.55.0 backup servers. |
There was a problem hiding this comment.
Is this still true? I thought that what the 'wire compatible embed version' stuff was about resolving?
There was a problem hiding this comment.
Wire version negotiation applies when connecting brokers (e.g., clustering), where both sides know each other's versions.
Journal persistence is different: when the backup reads journal records, it doesn't negotiate — it must handle whatever format was written. A 2.56.0+ version writing V4 records cannot be read by a pre-2.56.0 backup that only understands V3, hence forward-only compatibility.
There was a problem hiding this comment.
I will update teh version to 2.56 BTW in the comment.
There was a problem hiding this comment.
I think I have addressed your comment here. please Unresolve it if you still have questions?
There was a problem hiding this comment.
If its breaking compatibility then it seems like its going to need something in the documentation. At least the 'versions' bit, if not more, describing what folks would need to do to upgrade and that they then presumably could not downgrade without nuking any new persisted messages. Currently there seems to be nothing.
I've mentioned before that I'd also consider adding new persister formats without using them initially, then doing so later, so that at the switch-over some prior versions would already understand the new thing before its 'required'. I'd alternatively wonder about possibly allowing configuring it to use the older persisters to allow temporary compatibility for upgrades.
There was a problem hiding this comment.
Compatility is always forward. but I will add wording on the versions.
I don't think we should complicate this by allowing a previous version. as it would only complicate it further.
All you need is to do the right order on upgrades. Upgrade backup first, upgrade live next.
There was a problem hiding this comment.
I added stuff on versions.adoc, and I also added a section to rolling upgrade in ha.adoc.
I believe Red Hat already documents the rolling upgrade procedure in their downstream. I think it only makes sense to do the same upstream.
There was a problem hiding this comment.
BTW: I don't think this is breaking compatibility. Compatibility gurantees in the journal has always been forward only.
I have had a best effort to keep backward compatiblity as much as possible in the past, but it's never been a requirement.
|
Its now a lot closer to what I was suggesting several weeks back to get rid of the thread local and allow more direct and simpler testing of each part (arguably still outstanding). I was still thinking further that it would be possible to just have a single 'state' style object passed though as the processor/callback so startup only needed one of them for all messages rather than 1-per-message, though I can see that might be a bit awkward if different types of messages need a different set of state. |
|
Regarding the thread local: @gemmellr I couldn't get rid of an instance to hold intermediate state. I need to parse some data prior to create the message. Ideally we should read the data directly towards messages. I still need the intermediate state. and it's not easy to get rid of that. |
I didn't suggest getting rid of having an instance or intermediate state, more the opposite. I was just saying my original thought went a little bit further to e.g. having a singular intermediate state object passed around at startup for decoding, reusable for each message in turn (e.g much like the pool is reused that way) though I can see now that the shared persister API makes that a bit awkward for having different types of messages having different state (though one could argue they should all be pretty similar eventually). |
Say someone says it has 30K entries while the persister can only generate 8 I'm allowing 100 entries (which should be way beyond necessary to support newer versions).
Sizing affected how needsDepage() was returning true of false. I need to make sure I have the messages in memory on the selected queue
3 participants
No description provided.