Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

simplify TimeLimiter, fixing its tests #1088

Merged
merged 5 commits into from
Oct 23, 2018
Merged

simplify TimeLimiter, fixing its tests #1088

merged 5 commits into from
Oct 23, 2018

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Oct 10, 2018

  • previous implementation (introduced in Dont let pruning hold the index too long #1065) was a bit hastily implemented and
    much more complicated than needed:
    trying to support concurrency but not doing it well, while
    we don't actually need any of it.
  • unit testing was very flakey, in particular in circleCI there's
    lots of timing inaccuracy leading to broken tests
  • it was inaccurate, for shortlived operations it would be negligible
    but for longer operations it is not. While our use case operations
    should be short, it is nice to be able to fix this while we're at it.

So now we simplify the design a lot, basically reduce it to
a non-concurrent accounting structure, removing all channels,
goroutines and context, making the accounting more correct
and the testing more robust.

@Dieterbe Dieterbe mentioned this pull request Oct 10, 2018
* previous implementation was a bit hastily implemented and
  much more complicated than needed:
  trying to support concurrency but not doing it well, while
  we don't actually need any of it.
* unit testing was very flakey, in particular in circleCI there's
  lots of timing inaccuracy leading to broken tests
* it was inaccurate, for shortlived operations it would be negligible
  but for longer operations it is not. While our use case operations
  should be short, it is nice to be able to fix this while we're at it.

So now we simplify the design a lot, basically reduce it to
a non-concurrent accounting structure, removing all channels,
goroutines and context, making the accounting more correct
and the testing more robust.
@Dieterbe Dieterbe changed the title WIP: simplify TimeLimiter, fixing its tests simplify TimeLimiter, fixing its tests Oct 10, 2018
@Dieterbe
Copy link
Contributor Author

this fixes borken tests like

=== RUN   TestTimeLimiter
--- FAIL: TestTimeLimiter (0.00s)
    time_limit_test.go:19: scenario window 1: work done: 0 - wait should be 0. was supposed to take 0s, but took 157.989µs
FAIL
FAIL	github.com/grafana/metrictank/idx/memory	18.739s

e.g. as seen in unrelated PR's such as #1021

@Dieterbe
Copy link
Contributor Author

Dieterbe commented Oct 18, 2018

contrary to what i said in #1087 I now think we shouldn't add benchmarks to this, because:

  1. it's trivial to see that any Add() call will have very little overhead
  2. ditto for Wait()
  3. TimeLimiter states For correctness, you should always follow up an Add() with a Wait() so that's also how you should call it in benchmarks, but of course Waiting slows down the benchmarks based on what the limit was, rendering it pointless.

@Dieterbe Dieterbe requested a review from woodsaj October 18, 2018 17:52
// NewTimeLimiter creates a new TimeLimiter.
func NewTimeLimiter(window, limit time.Duration, now time.Time) *TimeLimiter {
if limit > window {
panic(fmt.Sprintf("NewTimeLimiter: invalid configuration. limit %s does not fit within window %s", limit, window))
Copy link
Member

Choose a reason for hiding this comment

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

dont panic, return an error. Let the caller decide if they want to panic or not.

@Dieterbe
Copy link
Contributor Author

@woodsaj PTAL

@Dieterbe Dieterbe merged commit 069b5ab into master Oct 23, 2018
@Dieterbe Dieterbe deleted the fix-time-limiter branch October 29, 2018 09:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants