You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/07/13 03:01:56 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
clebertsuconic opened a new pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879911427
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879823301
I believe we should follow what @tabish121 suggested on https://github.com/apache/activemq-artemis/pull/2975#discussion_r377327729
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit merged pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879898473
@franz1981 the information is already on the body... parsing it from somewhere else won't make it faster.. just parse the header upon reload. Adding more code will only make it worse.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-880458523
> 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 (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 the previous point)
The drawbacks are:
- backward compatibility must be accounted (not easy)
- 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 optimizations 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
- a persistent record can 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 OMM 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
clebertsuconic edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879898473
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-880359646
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879784321
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879899585
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879291925
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879911427
@clebertsuconic my point is more related to have a protocol agnostic way to save these bits that didn't need to know anything about the protocol and/or message content: 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-880458523
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879911427
@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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-880458523
> 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 just 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879911427
@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: that's why I think we should save any decoding if possible (in the best case scenario)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-880691282
@franz1981 that's a lot of change just for the header Franz ;) lets talk on next week
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879294010
@franz1981 I think we should always parse the header when reloading... Can't we revisit this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-880458523
> 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 just 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-880013982
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879911427
@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 that why I think we should save any decoding if possible (in the best case scenario)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-880458523
> 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?) that must be used to save any message decoding by accident (using the message) that contains all the info useful to the broker eg durability, routing type, address name...
- just store such properties on the message by populate them by using a "persistent header" section before the actual decoded message on the persistent medium
The drawbacks are:
- backward compatibility must be accounted (not easy)
- 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 optimizations 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
As @gemmellr says, using the header is doable although I suggest to perform a regression test with the reproducer of AMQP OMM 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879067048
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879911427
@clebertsuconic my point is more related to have a protocol agnostic way to save these bits, even if means having them twice: 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-880357531
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-879911427
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3650: ARTEMIS-3383 AMQPMessage.isDurable wrongly returns false during persistent lazy reload state
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3650:
URL: https://github.com/apache/activemq-artemis/pull/3650#issuecomment-880458523
> 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 just clearly state which properties can be used without causing any message decoding under the hood.
The drawbacks are:
- backward compatibility must be accounted (not easy)
- 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 optimizations 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
- a persistent record can 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 OMM 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org