From e5f5ca562f10727eead9e0de8fe1a090dea8399d Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Thu, 3 Dec 2015 12:29:24 +0100 Subject: [PATCH] SONAR-6995 CEQueueStatus get method should not modify state and they should not required the CEQueueStatusImpl object to be initialized either this way, monitoring can query the metrics of CEQueueStatus through JMX even before the object is initialized --- .../computation/monitoring/CEQueueStatus.java | 70 ++++++++++--------- .../monitoring/CEQueueStatusImpl.java | 65 +++++++++-------- .../CEQueueStatusImplConcurrentTest.java | 5 +- .../monitoring/CEQueueStatusImplTest.java | 56 +++++++-------- .../computation/queue/CeQueueImplTest.java | 6 ++ 5 files changed, 105 insertions(+), 97 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/monitoring/CEQueueStatus.java b/server/sonar-server/src/main/java/org/sonar/server/computation/monitoring/CEQueueStatus.java index d0e63ee1bfd..5f3ac4c2684 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/monitoring/CEQueueStatus.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/monitoring/CEQueueStatus.java @@ -21,21 +21,6 @@ package org.sonar.server.computation.monitoring; public interface CEQueueStatus { - /** - * Adds 1 to the count of received batch reports and 1 to the count of batch reports waiting for processing. - * - * @return the new count of received batch reports - * - * @see #getReceivedCount() - * @see #getPendingCount() - */ - long addReceived(); - - /** - * Count of received batch reports since instance startup - */ - long getReceivedCount(); - /** * Sets the count of reports waiting for processing at startup. This method can be called only once. * @@ -49,9 +34,16 @@ public interface CEQueueStatus { long initPendingCount(long initialPendingCount); /** - * Count of batch reports waiting for processing since startup, including reports received before instance startup. + * Adds 1 to the count of received batch reports and 1 to the count of batch reports waiting for processing. + * + * @return the new count of received batch reports + * + * @see #getReceivedCount() + * @see #getPendingCount() + * + * @throws IllegalStateException if {@link #initPendingCount(long)} has not been called yet */ - long getPendingCount(); + long addReceived(); /** * Adds 1 to the count of batch reports under processing and removes 1 from the count of batch reports waiting for @@ -61,13 +53,25 @@ public interface CEQueueStatus { * * @see #getInProgressCount() * @see #getPendingCount() + * + * @throws IllegalStateException if {@link #initPendingCount(long)} has not been called yet */ long addInProgress(); /** - * Count of batch reports under processing. + * Adds 1 to the count of batch reports which processing ended successfully and removes 1 from the count of batch + * reports under processing. Adds the specified time to the processing time counter. + * + * @param processingTime duration of processing in ms + * + * @return the new count of batch reports which processing ended successfully + * + * @see #getSuccessCount() + * @see #getInProgressCount() + * + * @throws IllegalArgumentException if processingTime is < 0 */ - long getInProgressCount(); + long addSuccess(long processingTime); /** * Adds 1 to the count of batch reports which processing ended with an error and removes 1 from the count of batch @@ -85,24 +89,24 @@ public interface CEQueueStatus { long addError(long processingTime); /** - * Count of batch reports which processing ended with an error since instance startup. + * Count of received batch reports since instance startup */ - long getErrorCount(); + long getReceivedCount(); /** - * Adds 1 to the count of batch reports which processing ended successfully and removes 1 from the count of batch - * reports under processing. Adds the specified time to the processing time counter. - * - * @param processingTime duration of processing in ms - * - * @return the new count of batch reports which processing ended successfully - * - * @see #getSuccessCount() - * @see #getInProgressCount() - * - * @throws IllegalArgumentException if processingTime is < 0 + * Count of batch reports waiting for processing since startup, including reports received before instance startup. */ - long addSuccess(long processingTime); + long getPendingCount(); + + /** + * Count of batch reports under processing. + */ + long getInProgressCount(); + + /** + * Count of batch reports which processing ended with an error since instance startup. + */ + long getErrorCount(); /** * Count of batch reports which processing ended successfully since instance startup. diff --git a/server/sonar-server/src/main/java/org/sonar/server/computation/monitoring/CEQueueStatusImpl.java b/server/sonar-server/src/main/java/org/sonar/server/computation/monitoring/CEQueueStatusImpl.java index fb03467ae25..2f67fddce20 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/computation/monitoring/CEQueueStatusImpl.java +++ b/server/sonar-server/src/main/java/org/sonar/server/computation/monitoring/CEQueueStatusImpl.java @@ -22,6 +22,7 @@ package org.sonar.server.computation.monitoring; import java.util.concurrent.atomic.AtomicLong; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkState; public class CEQueueStatusImpl implements CEQueueStatus { private static final long PENDING_INITIAL_VALUE = Long.MIN_VALUE; @@ -33,51 +34,33 @@ public class CEQueueStatusImpl implements CEQueueStatus { private final AtomicLong success = new AtomicLong(0); private final AtomicLong processingTime = new AtomicLong(0); - @Override - public long addReceived() { - // initPendingCount might not have been called yet - if (!pending.compareAndSet(PENDING_INITIAL_VALUE, 1)) { - pending.incrementAndGet(); - } - return received.incrementAndGet(); - } - - @Override - public long getReceivedCount() { - return received.get(); - } - @Override public long initPendingCount(long initialPendingCount) { checkArgument(initialPendingCount >= 0, "Initial pending count must be >= 0"); - if (!pending.compareAndSet(PENDING_INITIAL_VALUE, initialPendingCount)) { - throw new IllegalStateException("Method initPendingCount must be used before any other method and can not be called twice"); - } + checkState( + pending.compareAndSet(PENDING_INITIAL_VALUE, initialPendingCount), + "Method initPendingCount must be used before any other method and can not be called twice"); return initialPendingCount; } @Override - public long getPendingCount() { - ensurePendingIsInitialized(); + public long addReceived() { + ensurePendingInitialized("addReceived"); - return pending.get(); + pending.incrementAndGet(); + return received.incrementAndGet(); } @Override public long addInProgress() { - ensurePendingIsInitialized(); + ensurePendingInitialized("addInProgress"); pending.decrementAndGet(); return inProgress.incrementAndGet(); } - private void ensurePendingIsInitialized() { - pending.compareAndSet(PENDING_INITIAL_VALUE, 0); - } - - @Override - public long getInProgressCount() { - return inProgress.get(); + private void ensurePendingInitialized(String methodName) { + checkState(pending.get() != PENDING_INITIAL_VALUE, "Method initPendingCount must be used before %s can be called", methodName); } @Override @@ -87,11 +70,6 @@ public class CEQueueStatusImpl implements CEQueueStatus { return error.incrementAndGet(); } - @Override - public long getErrorCount() { - return error.get(); - } - @Override public long addSuccess(long processingTime) { addProcessingTime(processingTime); @@ -104,6 +82,27 @@ public class CEQueueStatusImpl implements CEQueueStatus { processingTime.addAndGet(time); } + @Override + public long getReceivedCount() { + return received.get(); + } + + @Override + public long getPendingCount() { + long currentValue = pending.get(); + return currentValue == PENDING_INITIAL_VALUE ? 0 : currentValue; + } + + @Override + public long getInProgressCount() { + return inProgress.get(); + } + + @Override + public long getErrorCount() { + return error.get(); + } + @Override public long getSuccessCount() { return success.get(); diff --git a/server/sonar-server/src/test/java/org/sonar/server/computation/monitoring/CEQueueStatusImplConcurrentTest.java b/server/sonar-server/src/test/java/org/sonar/server/computation/monitoring/CEQueueStatusImplConcurrentTest.java index 45ec82d267e..1096f432167 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/computation/monitoring/CEQueueStatusImplConcurrentTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/computation/monitoring/CEQueueStatusImplConcurrentTest.java @@ -49,6 +49,9 @@ public class CEQueueStatusImplConcurrentTest { @Test public void test_concurrent_modifications_in_any_order() throws InterruptedException { + long initialPendingCount = 9963L; + underTest.initPendingCount(initialPendingCount); + for (Runnable runnable : buildShuffleCallsToUnderTest()) { executorService.submit(runnable); } @@ -56,7 +59,7 @@ public class CEQueueStatusImplConcurrentTest { executorService.awaitTermination(1, TimeUnit.SECONDS); assertThat(underTest.getReceivedCount()).isEqualTo(100); - assertThat(underTest.getPendingCount()).isEqualTo(2); + assertThat(underTest.getPendingCount()).isEqualTo(initialPendingCount + 2); assertThat(underTest.getInProgressCount()).isEqualTo(1); assertThat(underTest.getErrorCount()).isEqualTo(17); assertThat(underTest.getSuccessCount()).isEqualTo(80); diff --git a/server/sonar-server/src/test/java/org/sonar/server/computation/monitoring/CEQueueStatusImplTest.java b/server/sonar-server/src/test/java/org/sonar/server/computation/monitoring/CEQueueStatusImplTest.java index 3d7f4c5e422..123b61a7290 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/computation/monitoring/CEQueueStatusImplTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/computation/monitoring/CEQueueStatusImplTest.java @@ -27,9 +27,9 @@ import org.junit.rules.ExpectedException; import static org.assertj.core.api.Assertions.assertThat; public class CEQueueStatusImplTest { - private static final String ISE_initPendingCount_CALL_MSG = "Method initPendingCount must be used before any other method and can not be called twice"; private static final int SOME_RANDOM_MAX = 96535; private static final int SOME_PROCESSING_TIME = 8723; + private static final long INITIAL_PENDING_COUNT = 996L; @Rule public ExpectedException expectedException = ExpectedException.none(); @@ -55,66 +55,60 @@ public class CEQueueStatusImplTest { @Test public void initPendingCount_throws_ISE_if_called_twice() { - expectISEForIllegalCallToInitPendingCount(); + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("Method initPendingCount must be used before any other method and can not be called twice"); underTest.initPendingCount(10); underTest.initPendingCount(10); } @Test - public void initPendingCount_throws_ISE_if_called_after_getPendingCount() { - expectISEForIllegalCallToInitPendingCount(); - - underTest.getPendingCount(); - underTest.initPendingCount(10); - } - - @Test - public void initPendingCount_throws_ISE_if_called_after_addReceived() { - expectISEForIllegalCallToInitPendingCount(); + public void addReceived_throws_ISE_if_called_before_initPendingCount() { + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("Method initPendingCount must be used before addReceived can be called"); underTest.addReceived(); - underTest.initPendingCount(10); - } - - @Test - public void initPendingCount_throws_ISE_if_called_after_addInProgress() { - expectISEForIllegalCallToInitPendingCount(); - - underTest.addInProgress(); - underTest.initPendingCount(10); - } - - private void expectISEForIllegalCallToInitPendingCount() { - expectedException.expect(IllegalStateException.class); - expectedException.expectMessage(ISE_initPendingCount_CALL_MSG); } @Test public void addReceived_sets_received_and_pending_counts_to_1_when_initPendingCount_has_not_been_called() { + underTest.initPendingCount(INITIAL_PENDING_COUNT); + underTest.addReceived(); assertThat(underTest.getReceivedCount()).isEqualTo(1); - assertThat(underTest.getPendingCount()).isEqualTo(1); + assertThat(underTest.getPendingCount()).isEqualTo(INITIAL_PENDING_COUNT + 1); } @Test public void addReceived_any_number_of_call_adds_1_per_call() { + underTest.initPendingCount(INITIAL_PENDING_COUNT); + int calls = new Random().nextInt(SOME_RANDOM_MAX); for (int i = 0; i < calls; i++) { underTest.addReceived(); } assertThat(underTest.getReceivedCount()).isEqualTo(calls); - assertThat(underTest.getPendingCount()).isEqualTo(calls); + assertThat(underTest.getPendingCount()).isEqualTo(INITIAL_PENDING_COUNT + calls); + } + + @Test + public void addInProgress_throws_ISE_if_called_before_initPendingCount() { + expectedException.expect(IllegalStateException.class); + expectedException.expectMessage("Method initPendingCount must be used before addInProgress can be called"); + + underTest.addInProgress(); } @Test public void addInProgress_increases_InProgress_and_decreases_Pending_by_1_without_check_on_Pending() { + underTest.initPendingCount(INITIAL_PENDING_COUNT); + underTest.addInProgress(); assertThat(underTest.getReceivedCount()).isEqualTo(0); - assertThat(underTest.getPendingCount()).isEqualTo(-1); + assertThat(underTest.getPendingCount()).isEqualTo(INITIAL_PENDING_COUNT - 1); assertThat(underTest.getInProgressCount()).isEqualTo(1); assertThat(underTest.getErrorCount()).isEqualTo(0); assertThat(underTest.getSuccessCount()).isEqualTo(0); @@ -123,13 +117,15 @@ public class CEQueueStatusImplTest { @Test public void addInProgress_any_number_of_call_change_by_1_per_call() { + underTest.initPendingCount(INITIAL_PENDING_COUNT); + int calls = new Random().nextInt(SOME_RANDOM_MAX); for (int i = 0; i < calls; i++) { underTest.addInProgress(); } assertThat(underTest.getInProgressCount()).isEqualTo(calls); - assertThat(underTest.getPendingCount()).isEqualTo(-calls); + assertThat(underTest.getPendingCount()).isEqualTo(INITIAL_PENDING_COUNT - calls); assertThat(underTest.getProcessingTime()).isEqualTo(0); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/computation/queue/CeQueueImplTest.java b/server/sonar-server/src/test/java/org/sonar/server/computation/queue/CeQueueImplTest.java index b2d3faef125..e291eae7539 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/computation/queue/CeQueueImplTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/computation/queue/CeQueueImplTest.java @@ -20,6 +20,7 @@ package org.sonar.server.computation.queue; import com.google.common.base.Optional; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -58,6 +59,11 @@ public class CeQueueImplTest { CeQueueListener listener = mock(CeQueueListener.class); CeQueue underTest = new CeQueueImpl(system2, dbTester.getDbClient(), uuidFactory, queueStatus, new CeQueueListener[] {listener}); + @Before + public void setUp() throws Exception { + queueStatus.initPendingCount(0); + } + @Test public void test_submit() { CeTaskSubmit.Builder submission = underTest.prepareSubmit(); -- 2.39.5