ISSUE #931,#907: Add option to track task execution time#932
Closed
athanatos wants to merge 1 commit intoapache:masterfrom
Closed
ISSUE #931,#907: Add option to track task execution time#932athanatos wants to merge 1 commit intoapache:masterfrom
athanatos wants to merge 1 commit intoapache:masterfrom
Conversation
eolivelli
requested changes
Jan 3, 2018
Contributor
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
Nice change. I left some minor comments
| } | ||
| } | ||
|
|
||
| @Test(timeout = 6000) |
Contributor
There was a problem hiding this comment.
Nit: Drop timeouts we have now in surefire config
| "BookieLongPollThread-" + serverCfg.getBookiePort(), OrderedScheduler.NO_TASK_LIMIT); | ||
| "BookieLongPollThread", | ||
| OrderedScheduler.NO_TASK_LIMIT, | ||
| NullStatsLogger.INSTANCE); |
Contributor
There was a problem hiding this comment.
Can't we pass the statslogger here as well?
Author
There was a problem hiding this comment.
Yeah, no reason not to. Will fix.
| ++toRemoveIndex; | ||
| } | ||
| if (toRemove != null) { | ||
| stopAutoRecoveryService(toRemove); |
Contributor
There was a problem hiding this comment.
This line seems to related to the issue
Contributor
There was a problem hiding this comment.
Sorry for typo, it does not seem to be related to the issue
Author
There was a problem hiding this comment.
stopAutoRecoveryService gets called as part of of killBookie. I refactored it a little to make the bs/bsLoggers state maintenance slightly less error prone.
Fixes a bug in OrderedScheduler introduced in e33ec10 which failed to track execution time with some callsand adds an option to enable it in the bookie. Also fix bug with task_queued duration. Add a simple mock for remembering stats long enough to verify that counters are actually used and sensible in unit tests and bake it into BookKeeperClusterTestCase so that we can write tests to ensure that the stats are actually counted and make sense. Use said mock to add simple tests for top level read and write stats validating this fix. (@bug W-4276826@) (@bug W-4268290@) Signed-off-by: Samuel Just <sjust@salesforce.com>
athanatos
force-pushed
the
forupstream/issue-931
branch
from
b00a3de to
dd15a5d
Compare
Author
|
@eolivelli I think I've addressed your other comments. |
sijie
pushed a commit
that referenced
this pull request
Jan 5, 2018
Fixes a bug in OrderedScheduler introduced in e33ec10 which failed to track execution time with some calls and adds an option to enable it in the bookie. Also fixes a bug with task_queued duration. Add a simple mock for remembering stats long enough to verify that counters are actually used and sensible in unit tests and bake it into BookKeeperClusterTestCase so that we can write tests to ensure that the stats are actually counted and make sense. Use said mock to add simple tests for top level read and write stats validating this fix. (bug W-4276826) (bug W-4268290) Signed-off-by: Samuel Just <sjustsalesforce.com> Author: Samuel Just <sjust@salesforce.com> Reviewers: Enrico Olivelli <eolivelli@gmail.com>, Sijie Guo <sijie@apache.org> This closes #932 from athanatos/forupstream/issue-931, closes #931, closes #907
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a bug in OrderedScheduler introduced in
e33ec10 which failed to track execution
time with some calls and adds an option to enable it in the bookie. Also
fixes a bug with task_queued duration.
Add a simple mock for remembering stats long enough to verify that
counters are actually used and sensible in unit tests and bake it into
BookKeeperClusterTestCase so that we can write tests to ensure that the
stats are actually counted and make sense. Use said mock to add simple
tests for top level read and write stats validating this fix.
(@bug W-4276826@)
(@bug W-4268290@)
Signed-off-by: Samuel Just sjust@salesforce.com