Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ScheduledReporter.start() starts with undesired delay #999

Merged
merged 11 commits into from
Jan 9, 2017
Merged

ScheduledReporter.start() starts with undesired delay #999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
020448d
Fixed Javadoc comment
sofax Aug 16, 2016
90d0dc7
Added fix for GitHub issue 998
sofax Aug 16, 2016
474b4b7
Merge branch '3.2-development' into 3.2-development
sofax Jan 9, 2017
9a34950
Fix for GitHub issue #998
sofax Jan 9, 2017
59b3019
Added test cases for issue #998
sofax Jan 9, 2017
588214f
Changed order of arguments in newly added start() method
sofax Jan 9, 2017
3fea4ed
Merge branch '3.2-development' of https://github.com/sofax/metrics in…
sofax Jan 9, 2017
989d8e1
Removed unnecessary Thread.sleep() calls
sofax Jan 9, 2017
33c91f9
Removed duplicate method
sofax Jan 9, 2017
7acca8a
Fixed order of arguments (again)
sofax Jan 9, 2017
9e08cb2
Reverted previous change, because it was unrelated to issue 998
sofax Jan 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 34 additions & 16 deletions metrics-core/src/main/java/com/codahale/metrics/ScheduledReporter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,13 @@
import java.io.Closeable;
import java.util.Locale;
import java.util.SortedMap;
import java.util.concurrent.*;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicInteger;

import org.slf4j.Logger;
Expand Down Expand Up @@ -66,7 +72,7 @@ public Thread newThread(Runnable r) {
* reporter will report
* @param name the reporter's name
* @param filter the filter for which metrics to report
* @param rateUnit a unit of time
* @param rateUnit a unit of time
* @param durationUnit a unit of time
*/
protected ScheduledReporter(MetricRegistry registry,
Expand Down Expand Up @@ -128,20 +134,32 @@ protected ScheduledReporter(MetricRegistry registry,
* @param period the amount of time between polls
* @param unit the unit for {@code period}
*/
synchronized public void start(long period, TimeUnit unit) {
if (this.scheduledFuture != null) {
throw new IllegalArgumentException("Reporter already started");
}
this.scheduledFuture = executor.scheduleAtFixedRate(new Runnable() {
@Override
public void run() {
try {
report();
} catch (RuntimeException ex) {
LOG.error("RuntimeException thrown from {}#report. Exception was suppressed.", ScheduledReporter.this.getClass().getSimpleName(), ex);
}
}
}, period, period, unit);
public void start(long period, TimeUnit unit) {
start(period, period, unit);
}

/**
* Starts the reporter polling at the given period.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a simple unit test to ScheduledReporterTest? A simple test which verifies that the scheduler wakes up with a correct initial delay. Very similar to the pollsPeriodically test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. All those tests are a little bit unreliable though, as the executor services rely on the system time instead of using a dedicated (mockable) clock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, instead of waiting for the reporter to actually do its work, it would be much better to just verify that the method scheduleAtFixedRate() was called with the correct arguments. There is no need to test the executor itself, as that's a collaborator that has already been tested elsewhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, instead of waiting for the reporter to actually do its work, it would be much better to just verify that the method scheduleAtFixedRate() was called with the correct arguments. There is no need to test the executor itself, as that's a collaborator that has already been tested elsewhere.

Agrred. This would work, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the tests are not isolated. The collaborators (mocks) should be initialized per test method, not per class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gnnhhh ... my IDE is configured to replace ".*" imports with the explicit ones - but I did NOT want to push those changes, as they were not necessary for the fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test cases have been added.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gnnhhh ... my IDE is configured to replace ".*" imports with the explicit ones - but I did NOT want to push those changes, as they were not necessary for the fix.

I think expanding star imports is a good change, we can leave it.

*
* @param initialDelay the time to delay the first execution
* @param period the amount of time between polls
* @param unit the unit for {@code period}
*/
synchronized public void start(long initialDelay, long period, TimeUnit unit) {
if (this.scheduledFuture != null) {
throw new IllegalArgumentException("Reporter already started");
}

this.scheduledFuture = executor.scheduleAtFixedRate(new Runnable() {
@Override
public void run() {
try {
report();
} catch (RuntimeException ex) {
LOG.error("RuntimeException thrown from {}#report. Exception was suppressed.", ScheduledReporter.this.getClass().getSimpleName(), ex);
}
}
}, initialDelay, period, unit);
}

/**
Expand Down
20 changes: 19 additions & 1 deletion metrics-core/src/test/java/com/codahale/metrics/ScheduledReporterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public void setUp() throws Exception {
registry.register("meter", meter);
registry.register("timer", timer);

when(executor.scheduleAtFixedRate(any(Runnable.class), eq(200L), eq(200L), eq(TimeUnit.MILLISECONDS)))
when(executor.scheduleAtFixedRate(any(Runnable.class), any(Long.class), any(Long.class), eq(TimeUnit.MILLISECONDS)))
.thenReturn(scheduledFuture);
}

Expand All @@ -63,6 +63,24 @@ public void pollsPeriodically() throws Exception {
);
}

@Test
public void shouldUsePeriodAsInitialDelayIfNotSpecifiedOtherwise() throws Exception {
reporterWithCustomExecutor.start(200, TimeUnit.MILLISECONDS);

verify(executor, times(1)).scheduleAtFixedRate(
any(Runnable.class), eq(200L), eq(200L), eq(TimeUnit.MILLISECONDS)
);
}

@Test
public void shouldStartWithSpecifiedInitialDelay() throws Exception {
reporterWithCustomExecutor.start(350, 100, TimeUnit.MILLISECONDS);

verify(executor).scheduleAtFixedRate(
any(Runnable.class), eq(350L), eq(100L), eq(TimeUnit.MILLISECONDS)
);
}

@Test
public void shouldAutoCreateExecutorWhenItNull() throws Exception {
reporterWithNullExecutor.start(200, TimeUnit.MILLISECONDS);
Expand Down
2 changes: 1 addition & 1 deletion metrics-servlets/src/main/java/com/codahale/metrics/servlets/MetricsServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ protected void doGet(HttpServletRequest req,
}
resp.setHeader("Cache-Control", "must-revalidate,no-cache,no-store");
resp.setStatus(HttpServletResponse.SC_OK);

final OutputStream output = resp.getOutputStream();
try {
if (jsonpParamName != null && req.getParameter(jsonpParamName) != null) {
Expand Down