[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4526: Close stuck sessions

["gemmellr (via GitHub)" <gi...@apache.org>]

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: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org