[
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)
