[ https://issues.apache.org/jira/browse/ARTEMIS-4758?focusedWorklogId=917320&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-917320 ]
ASF GitHub Bot logged work on ARTEMIS-4758: ------------------------------------------- Author: ASF GitHub Bot Created on: 02/May/24 16:16 Start Date: 02/May/24 16:16 Worklog Time Spent: 10m Work Description: gemmellr commented on code in PR #4918: URL: https://github.com/apache/activemq-artemis/pull/4918#discussion_r1587899745 ########## artemis-server/src/main/java/org/apache/activemq/artemis/core/config/Configuration.java: ########## @@ -1509,4 +1509,32 @@ default String resolvePropertiesSources(String propertiesFileUrl) { void setManagementRbacPrefix(String prefix); + /** This configures the Mirror Ack Manager number of attempts on queues before trying page acks. + * It is not intended to be configured through the XML. + * The default value here is 5. */ + int getMirrorAckManagerMinQueueAttempts(); + + Configuration setMirrorAckManagerMinQueueAttempts(int minQueueAttempts); + + /** This configures the Mirror Ack Manager number of attempts on page acks. + * It is not intended to be configured through the XML. + * The default value here is 2. */ + int getMirrorAckManagerMaxPageAttempts(); + + Configuration setMirrorAckManagerMaxPageAttempts(int maxPageAttempts); + + /** This configures the interval in which the Mirror AckManager will retry acks when + * It is not intended to be configured through the XML. + * The default value here is 100, and this is in milliseconds. */ + int getMirrorAckManagerRetryDelay(); + + Configuration setMirrorAckManagerRetryDelay(int delay); + + /** Should Mirror use Page Transactions When target destinations is paging? + * When a target queue on the mirror is paged, the mirror will not record a page transaction for every message. + * The default is false, and the overhead of paged messages will be smaller, but there is a possibility of eventual duplicates in case of interrupted communication between the mirror source and target. + * If you set this to false there will be extra syncs and a record stored on the journal for the page-transaction additionally to the record in the page store. */ Review Comment: The 'default is false, and the overhead will be smaller'....but then, 'if you set this false, there will be \<extra overhead\>' ? Which is it? ########## artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/connect/mirror/MirrorTransaction.java: ########## @@ -49,4 +51,13 @@ protected synchronized void afterCommit(List<TransactionOperation> operationsToC } } + @Override + public boolean isAllowPageTransaction() { + return ignorePageTransaction; + } + + public MirrorTransaction setIgnorePageTransaction(boolean ignorePageTransaction) { + this.ignorePageTransaction = ignorePageTransaction; + return this; + } Review Comment: The naming of the methods, and the variable feels off. The variable defaults to false, so ignorePageTransaction=false, meaning the return of 'isAllowPageTransaction()' is false. So it 'isnt ignored', but it also 'isnt allowed'? Confusing. Is there a better name here for one of them to make clearer? ########## artemis-commons/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQScheduledComponent.java: ########## @@ -212,15 +212,19 @@ public long getPeriod() { } public synchronized ActiveMQScheduledComponent setPeriod(long period) { - this.period = period; - restartIfNeeded(); + if (this.period != period) { + this.period = period; + restartIfNeeded(); + } return this; } public synchronized ActiveMQScheduledComponent setPeriod(long period, TimeUnit unit) { - this.period = period; - this.timeUnit = unit; - restartIfNeeded(); + if (this.period != period && this.timeUnit != unit) { Review Comment: Should this not be using "||" rather than "&&" so that if _either_ the period or time units changes, it updates things. Though that itself still isnt quite foolproof either, as both changes could effectively cancel each other out...e.g 1 sec vs 1000 ms. Probably more of a corner case there. Should probably check the units aren't null.. ########## artemis-server/src/main/java/org/apache/activemq/artemis/core/transaction/Transaction.java: ########## @@ -109,4 +109,8 @@ enum State { /** To be used on control transactions that are meant as internal and don't really require a hard sync. */ Transaction setAsync(boolean async); + + default boolean isAllowPageTransaction() { + return true; + } Review Comment: Similar confusion that this code default is true, but the overriding method looks to get a field value thats false by default? Issue Time Tracking ------------------- Worklog Id: (was: 917320) Time Spent: 20m (was: 10m) > Hardening Mirroring > ------------------- > > Key: ARTEMIS-4758 > URL: https://issues.apache.org/jira/browse/ARTEMIS-4758 > Project: ActiveMQ Artemis > Issue Type: Improvement > Reporter: Clebert Suconic > Priority: Major > Time Spent: 20m > Remaining Estimate: 0h > > I have been extensively testing mirroring, and I'm hardening it as an overal > task, improving its behavior: > - Page Transactions on mirror target are now optional. > * If you had an interrupt mirror while the target destination was paging, > duplicate detection would be ineffective unless you used paged transactions > * Users can now configure the ack manager retries intervals. > Say you need some time to remove a consumer from a target mirror. The > delivering references would prevent acks from happening. You can allow bigger > retry intervals and number of retries by tinkiering with ack manager retry > parameters. > * The ackManager was only restarted when new acks were coming in. If you > stopped receiving acks on a target server and restarted that server with > pending acks, those acks would never be exercised. The AckManager retries are > now restarted as soon as the server is started. -- This message was sent by Atlassian Jira (v8.20.10#820010)