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?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
