ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state

[clebertsuconic]

No description provided.

[gemmellr]

Are non-persistent messages ever paged and able to get into the 'persistent lazy reloaded' state? (I dont know how those bits work, and unexpectedly reinstalling means I dont currently have a dev env to look)

[clebertsuconic]

@gemmellr that's right... non persistent messages if through the paging.

although I think this lazyReload thing only applies to journal. last time I checked, a full parsing will always happen after depaging.

[clebertsuconic]

@franz1981 I think we should always parse the header when reloading... Can't we revisit this?

[gemmellr]

Parsing the header on reload is what I was suggesting on the other PR (or, doing what large-message instances seem to and storing the durability in the record so its seen before the message itself).

(Still working on dev env setup yet so still dont know and cant look how paging vs journal actually works hehe)

[franz1981]

I believe we should follow what @tabish121 suggested on #2975 (comment)

And I see that what @gemmellr is suggesting could be part of this. Will come back on my desk next week and we can speak about how to get it without causing regressions on the improvements made for https://issues.apache.org/jira/browse/ARTEMIS-2617

[clebertsuconic]

@franz1981 the information is already on the message... parsing it from somewhere else won't make it faster.. just parse the header upon reload. Adding more code will only make it worse. The Header is not that expensive to parse.

[clebertsuconic]

the only thing I do on LargeMessage is to store the beginning of the message on the journal, and I parse the header from there.

There are other implications from not parsing the header.. like some counters were off after reloading.

[franz1981]

@clebertsuconic my point is more related to have a protocol agnostic way to save these bits, even if means having them twice (in the protocol agnostic part + encoded on msg): this would save any protocol decoding just for few broker-related properties

If we can save decoding of the full message body to happen I am still happy because this would save journal loading (that's a single threaded operation) to spend memory and time into useless message decoding

The Header is not that expensive to parse.

I got a reproducer of amqp journal loading OOM and we can test how good it is with it on my back..
Or we can use the JMH benchmarks to try it too.
The problem is not parsing but producing garbage that would fill the heap before messages will be consumed (the hash maps storing differently property keys and values): that's why I think we should save any decoding if possible (in the best case scenario)

[gemmellr]

I agree with Tim/Franz that having general protocol agnostic behaviour in this area (and others) would seem a good idea.

I also think that parsing the header should be doable...the header isnt a map, has a limited number of values, and is at the start of the message data if it is present at all.

[clebertsuconic]

@franz1981 how you would represent this ? You would have an if (pendingParser) return isDuravle from a different place. If not get it from the header ?

It's code for no gain. Just parse the header on load. It's pointless to store it somewhere else.

[clebertsuconic]

@franz1981 besides, there's no generic persistence between the protocols. Each protocol would have its own persister, meaning if you duplicated this on the AMQPPersister, you would just be duplicating the header somewhere else for no point.

Just Parse the header. I'm afraid the issue is not just with isDurable here. Better to parse the header as soon as you reload it.

[franz1981]

how you would represent this ? You would have an if (pendingParser) return isDuravle from a different place. If not get it from the header ?

Depends where it's used; we have 2 options here:

• create a separate protocol agnostic class (PersistentHeader/BrokerHeader?) that contains all the info useful to the broker re persistent states eg durability, routing type, address name and encode/decode such header with some binary protocol-agnostic format before the actual content
• just store such properties on the message instance (as it is right now), but populating them using a "persistent header" section before the actual decoded message on the persistent medium (same as the previous point)

I prefer the former option or just using any separate API that clearly state which properties can be used without causing any message decoding under the hood.

The drawbacks are:

• backward compatibility must be accounted (both API and encoding format)
• duplication of information (as you rightly said)

Benefits:

• no more specific message mid-states on the message source code: message is fully decoded only in very special cases, while is fully not-decoded for most of the use cases, including journal (re)loading
• new protocols can get the same loading optimization of core/amqp for free eg MQTT in the near feature, if we would like to make it a first citizen
• no more message decoding by accident: right now using the "wrong" property can cause decoding to be stealthy fired and that's not easy to enforce/test/document
• persisted record could be inspected without any message decoding

As @gemmellr says, using the header is doable although I suggest to perform a regression test with the reproducer of AMQP OOM journal loading I have mentioned (we can speak about this next week); I'm still happy about this solution, but given the above points I believe is not future proof as I would like.

[clebertsuconic]

@franz1981 that's a lot of change just for the header Franz ;) lets talk on next week