You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "clebertsuconic (via GitHub)" <gi...@apache.org> on 2023/05/18 20:55:17 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request, #4484: ARTEMIS-4285 Disable persistent redelivery on new brokers
clebertsuconic opened a new pull request, #4484:
URL: https://github.com/apache/activemq-artemis/pull/4484
(no comment)
--
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 #4484: ARTEMIS-4285 Disable persistent redelivery on new brokers
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4484:
URL: https://github.com/apache/activemq-artemis/pull/4484#issuecomment-1554547617
@gtully what about having a max-redelivery-persistence instead of a boolean? (or find me a better name).
I would set it as default 5.
--
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] gtully commented on pull request #4484: ARTEMIS-4285 Disable persistent redelivery on new brokers
Posted by "gtully (via GitHub)" <gi...@apache.org>.
gtully commented on PR #4484:
URL: https://github.com/apache/activemq-artemis/pull/4484#issuecomment-1554269382
there may be a middle ground here. in 5.x the feature is around depending on the redeliveryFlag, or jms.isRedelivered which is set if redeliveryCount > 0
the persisting is only done on the transition from 0->1 rather than on each increment. This is sufficient for the flag, if the redelivery flag is false, then you know this is the first dispatch, otherwise you may need to check for duplicates.
There may be some value in supporting the redelivery flag being persisted, but not persisting every redelivery to keep the count up to date.
see: https://github.com/apache/activemq/blob/cfbea60d6d4f934e7fbe85915183a2f211414b82/activemq-broker/src/main/java/org/apache/activemq/broker/region/RegionBroker.java#L637
--
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] tabish121 commented on a diff in pull request #4484: ARTEMIS-4285 Disable persistent redelivery on new brokers
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #4484:
URL: https://github.com/apache/activemq-artemis/pull/4484#discussion_r1199182684
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/config/ActiveMQDefaultConfiguration.java:
##########
@@ -249,7 +249,7 @@ public static String getDefaultHapolicyBackupStrategy() {
private static boolean DEFAULT_PERSIST_DELIVERY_COUNT_BEFORE_DELIVERY = false;
// True means that we will persist redelivery on the journal
Review Comment:
This comment seems outdated now
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/config/Configuration.java:
##########
@@ -139,12 +139,15 @@ public interface Configuration {
Configuration setPersistenceEnabled(boolean enable);
/**
- * Sets when redeliveries are persisted or not.
- * This will control if the delivery counter and redelivery delay should be persisted.
+ * Maximum number of redelivery records stored on the journal per message reference.
*/
- Configuration setPersistRedelivery(boolean enable);
+ Configuration setMaxRedeliveryRecords(int maxPersistRedelivery);
- boolean isPersistRedelivery();
+ /**
+ * Maximum number of redelivery reco
Review Comment:
seems like the sentence died abruptly
--
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 #4484: ARTEMIS-4285 Disable persistent redelivery on new brokers
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4484:
URL: https://github.com/apache/activemq-artemis/pull/4484#issuecomment-1554822749
@gtully done... the commit still separate.. but I will squash it into the first commit after some review.
--
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 a diff in pull request #4484: ARTEMIS-4285 Disable persistent redelivery on new brokers
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on code in PR #4484:
URL: https://github.com/apache/activemq-artemis/pull/4484#discussion_r1199205018
##########
artemis-core-client/src/main/java/org/apache/activemq/artemis/api/config/ActiveMQDefaultConfiguration.java:
##########
@@ -249,7 +249,7 @@ public static String getDefaultHapolicyBackupStrategy() {
private static boolean DEFAULT_PERSIST_DELIVERY_COUNT_BEFORE_DELIVERY = false;
// True means that we will persist redelivery on the journal
Review Comment:
thanks a lot!
--
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 a diff in pull request #4484: ARTEMIS-4285 Disable persistent redelivery on new brokers
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on code in PR #4484:
URL: https://github.com/apache/activemq-artemis/pull/4484#discussion_r1198320952
##########
docs/user-manual/en/configuration-index.md:
##########
@@ -210,6 +210,7 @@ name | node name; used in topology notifications if set. | n/a
[read-whole-page](paging.md) | If true the whole page would be read, otherwise just seek and read while getting message. | `false`
[paging-directory](paging.md#configuration)| the directory to store paged messages in. | `data/paging`
[persist-delivery-count-before-delivery](undelivered-messages.md#delivery-count-persistence) | True means that the delivery count is persisted before delivery. False means that this only happens after a message has been cancelled. | `false`
+[persist-redelivery](undelivered-messages.md#persist-redelivery) | True means that a journal update record is added every time a redelivery occurs. If your system is playing with short redeliveries this should be set to false (recommended value). | `true`
Review Comment:
@tabish121 I just simplified the statement.
I was going to complete removal as I don't see much value on keeping these records at all. (I don't think it's a big deal to always delivery the message right away or to reset the delivery count once the broker is restarted).. but I may get users to disagree with me.
if you could help me with a better wording?
--
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 #4484: ARTEMIS-4285 Disable persistent redelivery on new brokers
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic commented on PR #4484:
URL: https://github.com/apache/activemq-artemis/pull/4484#issuecomment-1554756483
I will set it to 1
--
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 merged pull request #4484: ARTEMIS-4285 Limit number of redelivery records
Posted by "clebertsuconic (via GitHub)" <gi...@apache.org>.
clebertsuconic merged PR #4484:
URL: https://github.com/apache/activemq-artemis/pull/4484
--
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] tabish121 commented on a diff in pull request #4484: ARTEMIS-4285 Disable persistent redelivery on new brokers
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #4484:
URL: https://github.com/apache/activemq-artemis/pull/4484#discussion_r1198305231
##########
docs/user-manual/en/undelivered-messages.md:
##########
@@ -31,6 +31,12 @@ fail or rollback. Without a delayed redelivery, the system can get into a
and delivery being re-attempted ad infinitum in quick succession,
consuming valuable CPU and network resources.
+#Persist Redelivery
+
+Two Journal update records are stored every time a redelivery happens. One for the number of deliveries that happened, and one in case a scheduled redelivery is being used.
+It is recommended to keep persist-redelivery=false as if you are playing with very short redeliveries you may create unecessary records on the journal.
Review Comment:
Reworded a bit and changed to 'redelivery delays' as long / short redeliveries doesn't really convey logical meaning
"It is recommended to keep persist-redelivery=false in situations where you are operating with very short redelivery delays as you will be creating unecessary records on the journal."
##########
docs/user-manual/en/configuration-index.md:
##########
@@ -210,6 +210,7 @@ name | node name; used in topology notifications if set. | n/a
[read-whole-page](paging.md) | If true the whole page would be read, otherwise just seek and read while getting message. | `false`
[paging-directory](paging.md#configuration)| the directory to store paged messages in. | `data/paging`
[persist-delivery-count-before-delivery](undelivered-messages.md#delivery-count-persistence) | True means that the delivery count is persisted before delivery. False means that this only happens after a message has been cancelled. | `false`
+[persist-redelivery](undelivered-messages.md#persist-redelivery) | True means that a journal update record is added every time a redelivery occurs. If your system is playing with short redeliveries this should be set to false (recommended value). | `true`
Review Comment:
The term 'short redeliveries' doesn't really read well to me, seems more accurate to write it as 'short redelivery delays' based on what I think you mean.
--
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