Move alternative implementations to internal/alternatives/* #37

rabbbit · 2020-10-04T23:54:48Z

We currently have two different implementations. Currently:

they're in the same package
one of them is currently not tested
one has a bug related to slack (see Need to get the last sleepFor in the loop. #27, use the old state's sleepFor #23)
one has a broken constructor taking options affecting the other type

We plan to add at least 3 more implementations:

one that addresses Evaluate atomic and mutex based Take implementations. #22
one that re-uses ratelimitfx atomic style
one that compares us against x/time/rate

Even if we decide to eventually keep a single implementation,
they'll be there for a while. And I imagine it'll get noisy.

(A crazy idea would be to return different implementation based on
some user params. But that's totally TBD).

This diff tries to organize us a bit my moving the separate
implementations to separate packages.

Followups:

will try to refactor the tests into a shared test package & re-use them.
This way we can write tests once and make sure new implementations
are working as expected.
will try to make sure the implementations take equivalent
configuration
will have to modify the root ratelimit package to have stronger
type guarantees, and not just aliasing methods - that would require
a separate /types package though (I think).

Accidentally this diff is also changing signature of AfterFunc
in internal/clock package. This is so that it implements
it's own Clock interface. It's a bit of a temporary hack
as well - #3 will make it a bit simpler.

We currently have two different implementations. Currently: - they're in the same package - one of them is currently not tested - one has a bug related to slack (see #27, #23) - one has a broken constructor taking options affecting the other type We plan to add at least 3 more implementations: - one that addresses #22 - one that re-uses ratelimitfx atomic style - one that compares us against x/time/rate Even if we decide to eventually keep a single implementation, they'll be there for a while. And I imagine it'll get noisy. (A crazy idea would be to return different implementation based on some user params. But that's totally TBD). This diff tries to organize us a bit my moving the separate implementations to separate packages. Followups: - will try to refactor the tests into a shared test package & re-use them. This way we can write tests once and make sure new implementations are working as expected. - will try to make sure the implementations take equivalent configuration - will have to modify the root ratelimit package to have stronger type guarantees, and not just aliasing methods - that would require a separate /types package though (I think). Accidentally this diff is also changing signature of AfterFunc in internal/clock package. This is so that it implements it's own Clock interface. Looking at #3 though, I think we can simply drop AfterFunc from the interface in followup diffs, and re-use Clock from the root ratelimit package. (We currently have two Clock interfaces defined, we can merge them into a single one using a /types package trick) in a follow up.

rabbbit · 2020-10-05T17:31:52Z

About

Accidentally this diff is also changing signature of AfterFunc
in internal/clock package. This is so that it implements
it's own Clock interface. It's a bit of a temporary hack
as well - #3 will make it a bit simpler.

I tried to fix #3 in a separate PR, got stuck at something, will try again.
So we could potentially wait with landing this one.

But I'd still like to hear your thoughts on the idea:)

This came up in #37 where we ended up in a dependency loop. - main package was importing a specific rate limiter type - both rate limiters needed to import Clock types from main package By moving it to a separate package we avoid that loop. If we decide to go ahead with #37, this will also be necessary to have a "common config type" that can be re-used between all N alternative limiters.

Resolves #3 Was looking at the open issues and #3 seemed to make sense - it was also something that I saw in #37 so I thought I'd take a stab. The reasoning is that less code + smaller interface is a good thing. During the diff it turned out that: - our internal/clock has no tests - our internal/clock has at least two bugs: -- one that I'm fixing here, related to not executing two timers if they're scheduled at the "now" boundry -- one regarding adding time but not having a timer set for that specific time (not a problem for us, so not touching it) Looking at the internal/clock, we see: ``` // Forked from github.com/andres-erbsen/clock to isolate a missing nap. ``` There's one extra nap in AfterFunc that I'm removing in this diff, and so I'm hoping that by removing AfterFunc we'll eventually be able to move off the fork. The tests are a bit of a pain, I'll put up another PR for a test runner that will make this a bit easier.

Resolves #3 Was looking at the open issues and #3 seemed to make sense - it was also something that I saw in #37 so I thought I'd take a stab. The reasoning is that less code + smaller interface is a good thing. During the diff it turned out that: - our internal/clock has no tests - our internal/clock has at least two bugs: -- one that I'm fixing here, related to not executing two timers if they're scheduled at the "now" boundry -- one regarding adding time but not having a timer set for that specific time (not a problem for us, so not touching it) Looking at the internal/clock, we see: ``` // Forked from github.com/andres-erbsen/clock to isolate a missing nap. ``` There's one extra nap in AfterFunc that I'm removing in this diff, and so I'm hoping that by removing AfterFunc we'll eventually be able to move off the fork.

…39) * Get rid of internal/clock #1 - Consolidate AfterFunc and Sleep in tests Resolves #3 Was looking at the open issues and #3 seemed to make sense - it was also something that I saw in #37 so I thought I'd take a stab. The reasoning is that less code + smaller interface is a good thing. During the diff it turned out that: - our internal/clock has no tests - our internal/clock has at least two bugs: -- one that I'm fixing here, related to not executing two timers if they're scheduled at the "now" boundry -- one regarding adding time but not having a timer set for that specific time (not a problem for us, so not touching it) Looking at the internal/clock, we see: ``` // Forked from github.com/andres-erbsen/clock to isolate a missing nap. ``` There's one extra nap in AfterFunc that I'm removing in this diff, and so I'm hoping that by removing AfterFunc we'll eventually be able to move off the fork.

rabbbit · 2020-12-06T00:44:57Z

Don't think we'll be doing this in the end.

rabbbit assigned cinchurge Oct 5, 2020

rabbbit mentioned this pull request Oct 7, 2020

Prefactor types into a separarate types package #38

Closed

rabbbit mentioned this pull request Oct 19, 2020

Get rid of internal/clock - Consolidate AfterFunc and Sleep in tests #39

Merged

rabbbit closed this Dec 6, 2020

rabbbit deleted the pawel/alt branch January 19, 2021 02:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move alternative implementations to internal/alternatives/* #37

Move alternative implementations to internal/alternatives/* #37

rabbbit commented Oct 4, 2020 •

edited

rabbbit commented Oct 5, 2020

rabbbit commented Dec 6, 2020

Move alternative implementations to internal/alternatives/* #37

Move alternative implementations to internal/alternatives/* #37

Conversation

rabbbit commented Oct 4, 2020 • edited

rabbbit commented Oct 5, 2020

rabbbit commented Dec 6, 2020

rabbbit commented Oct 4, 2020 •

edited