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

Fix SlidingTimeWindowReservoir trim operation after overflow #1063

Merged
merged 2 commits into from Feb 7, 2017

Conversation

Projects
None yet
3 participants
@ho3rexqj
Contributor

ho3rexqj commented Feb 4, 2017

The existing trim operation in SlidingTimeWindowReservoir assumes that values returned by getTick are strictly increasing. When the result returned by getTick overflows, the trim operation will behave improperly - trim operations just after the overflow event will likely clear values within the current window, and trim operations well past the overflow event will likely fail to clear updates outside the current window (updates added before the overflow event).

To illustrate this behavior consider a reservoir with an 8-bit value returned by getTick, a window size of 2, and ignore the trim operations effect on the tick count:

update @123: 1
update @124: 2
update @125: 3
update @126: 4
update @127: 5
trim @127 - clears values before @125 (1, 2)
update @-128: 6
trim @-128 - clears values before @126 (3, 6) - 6 cleared improperly
update @-127: 7
update @-126: 8
trim @-126 - clears values before @-128 (none) - 4,5 "leaked"

The proposed fix properly clears any values in the reservoir outside of the sliding time window. For the purposes of that operation, the start of the time window is defined as now-window, and the end of the time window is defined as now+CLEAR_BUFFER, where now is the value returned by getTick at the start of the trim operation. This prevents trim from improperly clearing updates added concurrently while the operation is underway.

ho3rexqj added some commits Feb 4, 2017

Updated SlidingTimeWindowReservoir's trim operation to properly clear…
… all existing values outside of sliding window. Note that in this context the time window is defined as "now-window" to "now+CLEAR_BUFFER", where "CLEAR_BUFFER" is a sufficiently large to exclude any updates pushed to the reservoir while the trim operation is underway.

@jplock jplock added this to the 3.2.0 milestone Feb 7, 2017

@arteam arteam merged commit e798206 into dropwizard:3.2-development Feb 7, 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 Feb 7, 2017

Member

Thank you very much for the very detailed write-up and the overflow test!

Member

arteam commented Feb 7, 2017

Thank you very much for the very detailed write-up and the overflow test!

arteam added a commit that referenced this pull request Feb 7, 2017

Fix SlidingTimeWindowReservoir trim operation after overflow (#1063)
* Added a SlidingTimeWindowReservoir test illustrating the overflow failure.

* Updated SlidingTimeWindowReservoir's trim operation to properly clear all existing values outside of sliding window.  Note that in this context the time window is defined as "now-window" to "now+CLEAR_BUFFER", where "CLEAR_BUFFER" is a sufficiently large to exclude any updates pushed to the reservoir while the trim operation is underway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment