[
https://issues.apache.org/jira/browse/ARTEMIS-5001?focusedWorklogId=931732&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-931732
]
ASF GitHub Bot logged work on ARTEMIS-5001:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 26/Aug/24 14:22
Start Date: 26/Aug/24 14:22
Worklog Time Spent: 10m
Work Description: gemmellr commented on code in PR #5164:
URL: https://github.com/apache/activemq-artemis/pull/5164#discussion_r1731260945
##########
artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/OperationConsistencyLevel.java:
##########
@@ -0,0 +1,21 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.activemq.artemis.core.io;
+
+public enum OperationConsistencyLevel {
+ full, storage, ignoreReplication
Review Comment:
Usually uppercase
##########
tests/soak-tests/src/test/java/org/apache/activemq/artemis/tests/soak/brokerConnection/mirror/ReplicatedBothNodesMirrorTest.java:
##########
@@ -163,7 +166,7 @@ private static void createMirroredServer(String serverName,
HelperCreate cliCreateServer = new HelperCreate();
cliCreateServer.setAllowAnonymous(true).setArtemisInstance(serverLocation);
- cliCreateServer.setNoWeb(true);
+ cliCreateServer.setNoWeb(false);
Review Comment:
Intended? If so could the line just be removed instead?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/OperationContextImpl.java:
##########
@@ -201,56 +193,80 @@ public synchronized void replicationDone() {
@Override
public void executeOnCompletion(IOCallback runnable) {
- executeOnCompletion(runnable, false);
+ executeOnCompletion(runnable, OperationConsistencyLevel.full);
}
@Override
- public void executeOnCompletion(final IOCallback completion, final boolean
storeOnly) {
+ public void executeOnCompletion(final IOCallback completion, final
OperationConsistencyLevel consistencyLevel) {
boolean executeNow = false;
synchronized (this) {
if (errorCode == -1) {
final long storeLined = STORE_LINEUP_UPDATER.get(this);
final long pageLined = PAGE_LINEUP_UPDATER.get(this);
final long replicationLined = REPLICATION_LINEUP_UPDATER.get(this);
- if (storeOnly) {
- if (storeOnlyTasks == null) {
- storeOnlyTasks = new LinkedList<>();
- }
- } else {
- if (tasks == null) {
- tasks = new LinkedList<>();
- minimalReplicated = replicationLined;
- minimalStore = storeLined;
- minimalPage = pageLined;
- }
- }
- // On this case, we can just execute the context directly
-
- if (replicationLined == replicated && storeLined == stored &&
pageLined == paged) {
- // We want to avoid the executor if everything is complete...
- // However, we can't execute the context if there are
executions pending
- // We need to use the executor on this case
- if (EXECUTORS_PENDING_UPDATER.get(this) == 0) {
- // No need to use an executor here or a context switch
- // there are no actions pending.. hence we can just execute
the task directly on the same thread
- executeNow = true;
- } else {
- execute(completion);
- }
- } else {
- if (storeOnly) {
- if (storeLined == stored &&
EXECUTORS_PENDING_UPDATER.get(this) == 0) {
- executeNow = true;
+ switch (consistencyLevel) {
+ case storage:
+ if (storeOnlyTasks == null) {
+ storeOnlyTasks = new LinkedList<>();
+ if (minimalStore == Long.MAX_VALUE) minimalStore =
storeLined;
Review Comment:
Lets use braces (even if on same line) to avoid issues later. Also, given
the old code didnt seem to do this, maybe explain what its doing?
Same for the related cases below.
##########
tests/soak-tests/src/test/java/org/apache/activemq/artemis/tests/soak/brokerConnection/mirror/ReplicatedBothNodesMirrorTest.java:
##########
@@ -185,20 +188,25 @@ private static void createMirroredServer(String
serverName,
brokerProperties.put("AMQPConnections." + connectionName +
".connectionElements.mirror.sync", "false");
brokerProperties.put("largeMessageSync", "false");
- brokerProperties.put("addressSettings.#.maxSizeMessages", "50");
- brokerProperties.put("addressSettings.#.maxReadPageMessages", "2000");
- brokerProperties.put("addressSettings.#.maxReadPageBytes", "-1");
- brokerProperties.put("addressSettings.#.prefetchPageMessages", "500");
+ brokerProperties.put("mirrorAckManagerQueueAttempts", "5");
+ brokerProperties.put("mirrorAckManagerPageAttempts", "500000");
Review Comment:
Could this be set in the later 'if (paging)' block ?
##########
tests/soak-tests/src/test/java/org/apache/activemq/artemis/tests/soak/brokerConnection/mirror/ReplicatedBothNodesMirrorTest.java:
##########
@@ -65,7 +68,7 @@ public class ReplicatedBothNodesMirrorTest extends
SoakTestBase {
private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
// Set this to true and log4j will be configured with some relevant
log.trace for the AckManager at the server's
- private static final boolean TRACE_LOGS =
Boolean.parseBoolean(TestParameters.testProperty(TEST_NAME, "TRACE_LOGS",
"false"));
+ private static final boolean TRACE_LOGS =
Boolean.parseBoolean(TestParameters.testProperty(TEST_NAME, "TRACE_LOGS",
"true"));
Review Comment:
Intended for commit?
##########
tests/soak-tests/src/test/java/org/apache/activemq/artemis/tests/soak/brokerConnection/mirror/ReplicatedBothNodesMirrorTest.java:
##########
@@ -213,15 +221,17 @@ private static void replaceLogs(File serverLocation)
throws Exception {
"logger.artemis_utils.level=INFO\n" +
"\n" +
"logger.endpoint.name=org.apache.activemq.artemis.core.replication.ReplicationEndpoint\n"
+
"logger.endpoint.level=INFO\n" +
+
"logger.ack.name=org.apache.activemq.artemis.protocol.amqp.connect.mirror.AckManager\n"
+
+ "logger.ack.level=TRACE\n" +
Review Comment:
Running a soak with trace logs seems a bit weird...intended?
##########
artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/collections/HashMapStorageManager.java:
##########
@@ -0,0 +1,20 @@
+package org.apache.activemq.artemis.core.journal.collections;
+
+import org.apache.activemq.artemis.core.journal.IOCompletion;
+import org.apache.activemq.artemis.core.persistence.Persister;
+
+public interface HashMapStorageManager {
+ void storeMapEntry(long id,
Review Comment:
What about storeMapRecord? Would fit with the parameters better, plus
'entry' tends more to be used to describe things inside the map rather than the
whole map, so this feels a little weird.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/OperationContextImpl.java:
##########
@@ -48,17 +49,7 @@
public class OperationContextImpl implements OperationContext {
- private boolean syncReplication = true;
-
- @Override
- public void setSyncReplication(boolean syncReplication) {
- this.syncReplication = syncReplication;
- }
-
- @Override
- public boolean isSyncReplication() {
- return syncReplication;
- }
+ private boolean relaxedReplication = true;
Review Comment:
Is this used?
##########
artemis-journal/src/main/java/org/apache/activemq/artemis/core/journal/collections/HashMapStorageManager.java:
##########
@@ -0,0 +1,20 @@
+package org.apache.activemq.artemis.core.journal.collections;
+
+import org.apache.activemq.artemis.core.journal.IOCompletion;
+import org.apache.activemq.artemis.core.persistence.Persister;
+
+public interface HashMapStorageManager {
Review Comment:
Perhaps the more general _Map_ rather than _HashMap_? Or maybe _MapRecord_
since that seems to be what is passed in?
Issue Time Tracking
-------------------
Worklog Id: (was: 931732)
Time Spent: 1h 50m (was: 1h 40m)
> Add configuration option to relax syncs journal replication for Mirror Target
> -----------------------------------------------------------------------------
>
> Key: ARTEMIS-5001
> URL: https://issues.apache.org/jira/browse/ARTEMIS-5001
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Affects Versions: 2.37.0
> Reporter: Clebert Suconic
> Assignee: Clebert Suconic
> Priority: Major
> Fix For: 2.38.0
>
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> When I worked on AMQP Mirror I did not actually envision being used with
> journal replication. I actually thought more about adding multiple mirrored
> options instead.
> However an user reported me that when using mirror and journal replication
> combined, the sends could take a lot longer to happen (some normal latency)
> and the acks would eventually be missed.
> I should add an option to ignore the replication for the Mirror Target.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
