gemmellr commented on code in PR #4526:
URL: https://github.com/apache/activemq-artemis/pull/4526#discussion_r1244935699
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/persistence/impl/journal/OperationContextUnitTest.java:
##########
@@ -422,4 +422,37 @@ public void onError(final int errorCode, final String
errorMessage) {
Assert.assertEquals(0, operations.get());
}
+ @Test
+ public void testContextWithoutStoreOperationTrackers() {
+ ExecutorService executor =
Executors.newSingleThreadExecutor(ActiveMQThreadFactory.defaultThreadFactory(getClass().getName()));
+
+ OperationContextImpl.setMaxStoreOperationTrackers(0);
+ OperationContextImpl context = new OperationContextImpl(executor);
+ Assert.assertNull(context.getStoreOperationTrackers());
+
+ context.storeLineUp();
+ Assert.assertNull(context.getStoreOperationTrackers());
+
+ context.done();
+ Assert.assertNull(context.getStoreOperationTrackers());
+ }
+
+ @Test
+ public void testContextWithStoreOperationTrackers() {
+ ExecutorService executor =
Executors.newSingleThreadExecutor(ActiveMQThreadFactory.defaultThreadFactory(getClass().getName()));
+
+ OperationContextImpl.setMaxStoreOperationTrackers(1);
Review Comment:
This sets a static, but it doesnt seem like it is reset later, so does it
remain set for all subsequent tests?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/persistence/impl/journal/OperationContextImpl.java:
##########
@@ -415,4 +449,27 @@ public String toString() {
executorsPendingField +
"]";
}
+
+ @Override
+ public void clear() {
+ stored = 0;
+ storeLineUpField = 0;
+ minimalReplicated = 0;
+ replicated = 0;
+ replicationLineUpField = 0;
+ paged = 0;
+ minimalPage = 0;
+ pageLineUpField = 0;
+ errorCode = -1;
+ errorMessage = null;
+ executorsPendingField = 0;
+
+ if (tasks != null) {
+ tasks.clear();
+ }
+
+ if (storeOnlyTasks != null) {
+ storeOnlyTasks.clear();
+ }
Review Comment:
These lists are not thread-safe, and this could potentially be a concurrent
operation (people can tend to use 'force' toggles even if they dont actually
need to). Was this method meant to be synchronized, or did you not want to
synchronize it (although setting the other fields also wont really be safe
without doing so..) ? If the latter do they need changed to a concurrent
structure, or is it ok they may get corrupted in this scenario?
##########
artemis-server/src/test/java/org/apache/activemq/artemis/core/persistence/impl/journal/OperationContextUnitTest.java:
##########
@@ -422,4 +422,37 @@ public void onError(final int errorCode, final String
errorMessage) {
Assert.assertEquals(0, operations.get());
}
+ @Test
+ public void testContextWithoutStoreOperationTrackers() {
+ ExecutorService executor =
Executors.newSingleThreadExecutor(ActiveMQThreadFactory.defaultThreadFactory(getClass().getName()));
+
+ OperationContextImpl.setMaxStoreOperationTrackers(0);
Review Comment:
Rather than set it 0, could it just verify it is 0, i.e test the 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]
