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.

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

Conversation

Projects
None yet
2 participants
@sofax
Contributor

sofax commented Aug 16, 2016

ScheduledReporter.start() incorrectly uses the interval size for the start delay, too:

    public void start(long period, TimeUnit unit) {
        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);
    }

This is a serious problem when the interval is sufficiently big. I suggest to add an alternative version of ScheduledReporter.start() to fix the bug without affecting code that relies on this behavior:

    public void start(long period, long initialDelay, TimeUnit unit) {
        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);
    }

@ryantenney ryantenney changed the title from Pull request for https://github.com/dropwizard/metrics/issues/998 to ScheduledReporter.start() starts with undesired delay Aug 16, 2016

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Jan 5, 2017

Member

Hello @sofax,

Sorry to get back to you late. I like your pull request, could you please rebase it against the 3.2-development branch? Due changes from another pull requests (respectively #1018), it now has merge conflicts and can't be merged. Thank you.

Member

arteam commented Jan 5, 2017

Hello @sofax,

Sorry to get back to you late. I like your pull request, could you please rebase it against the 3.2-development branch? Due changes from another pull requests (respectively #1018), it now has merge conflicts and can't be merged. Thank you.

@arteam

Thanks for actualizing your changes! I've added a couple of comments regarding the implementation.

@arteam arteam removed the unable to merge label Jan 9, 2017

@arteam arteam added this to the 3.2.0 milestone Jan 9, 2017

@arteam

arteam approved these changes Jan 9, 2017

@arteam arteam merged commit 0ccce2c into dropwizard:3.2-development Jan 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Jan 9, 2017

Member

Thank you for your contribution!

Member

arteam commented Jan 9, 2017

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment