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

Jump to bottom

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 3 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions metrics-core/src/main/java/com/codahale/metrics/ScheduledReporter.java
View file
Open in desktop
Expand Up @@ -144,6 +144,26 @@ public void run() {
}, 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 period the amount of time between polls
* @param initialDelay the time to delay the first execution
* @param unit the unit for {@code period}
*/
public void start(long period, long initialDelay, TimeUnit unit) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to change the order of parameters, so initialDelay comes first, to maintain the same order as in the scheduleAtFixedRate method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. :)

executor.scheduleAtFixedRate(new Runnable() {
Copy link
Member

Choose a reason for hiding this comment

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

I think we could avoid duplication and implement the method as:

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);
   }

as then make the start(long period, TimeUnit unit) method to delegate to it as

start(period, period, unit)

Copy link
Contributor Author

@sofax sofax Jan 9, 2017 •
edited

Choose a reason for hiding this comment

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

Again, agreed.

The lines

        if (this.scheduledFuture != null) {
            throw new IllegalArgumentException("Reporter already started");
        }

are new, but they do look reasonable to me.

@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);
}

/**
* Stops the reporter and if shutdownExecutorOnStop is true then shuts down its thread of execution.
*
Expand Down
6 changes: 6 additions & 0 deletions metrics-servlets/src/main/java/com/codahale/metrics/servlets/MetricsServlet.java
View file
Open in desktop
Expand Up @@ -64,6 +64,9 @@ protected String getAllowedOrigin() {

/**
* Returns the name of the parameter used to specify the jsonp callback, if any.
*
Copy link
Member

Choose a reason for hiding this comment

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

It seems to me as an unrelated change. Could you please revert it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, already working on it - it got pushed too soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change has been reverted.

* @return the name of the parameter used to specify the jsonp callback, or <code>null</code>, if
* no such parameter exists.
*/
protected String getJsonpCallbackParameter() {
return null;
Expand All @@ -72,6 +75,9 @@ protected String getJsonpCallbackParameter() {
/**
* Returns the {@link MetricFilter} that shall be used to filter metrics, or {@link MetricFilter#ALL} if
* the default should be used.
*
* @return the {@link MetricFilter} that shall be used to filter metrics, or {@link MetricFilter#ALL} if
* the default should be used.
*/
protected MetricFilter getMetricFilter() {
// use the default
Expand Down