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.

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 #998

Closed
sofax opened this issue Aug 16, 2016 · 4 comments
Closed

ScheduledReporter.start() starts with undesired delay #998

sofax opened this issue Aug 16, 2016 · 4 comments

Comments

@sofax
Copy link
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);
    }
@sofax
Copy link
Contributor Author

sofax commented Aug 16, 2016

I added the suggested fix to the fork
https://github.com/sofax/metrics

@sofax
Copy link
Contributor Author

sofax commented Aug 16, 2016

This is the pull request for it:
#999

@sofax sofax closed this as completed Aug 16, 2016
@sofax sofax reopened this Aug 16, 2016
@ryantenney
Copy link
Contributor

Will track in #999

sofax added a commit to sofax/metrics that referenced this issue Jan 9, 2017
sofax added a commit to sofax/metrics that referenced this issue Jan 9, 2017
arteam pushed a commit that referenced this issue Jan 9, 2017
@arteam
Copy link
Member

arteam commented Jan 9, 2017

Closed via #999

@arteam arteam reopened this Jan 9, 2017
@arteam arteam closed this as completed Feb 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants