Get rid of internal/clock - Consolidate AfterFunc and Sleep in tests #39
Conversation
This file contains 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
rabbbit
commented
Oct 19, 2020
rabbbit
commented
Oct 19, 2020
Merged
rabbbit
added a commit
that referenced
this pull request
Oct 26, 2020
rabbbit
force-pushed
the
pawel/afterfunc
branch
3 times, most recently
from
0f0117b
to
8f0d35a
Compare
|
Rebased it on top of #40 and it looks much better now. I'll raise another PR trying to switch us to andres-erbsen/clock in parallel to make sure we cannot simply do that, without even landing this diff. |
|
Raised #41 to test - verified that:
|
rabbbit
force-pushed
the
pawel/afterfunc
branch
from
8f0d35a
to
529e3ae
Compare
|
change overall looks good, but i think we should split it up into 3 separate changes:
what other work is needed in order to move off of the fork? |
I'm happy to split the fixes, but I won't be adding tests for them - since we're this diff away from removing the fork. The work needed - if this lands, we can drop the fork. |
rabbbit
force-pushed
the
pawel/afterfunc
branch
from
529e3ae
to
8cf7737
Compare
rabbbit
changed the title
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.
Was untested and contained bugs.
rabbbit
force-pushed
the
pawel/afterfunc
branch
from
8cf7737
to
d2b91bb
Compare
cinchurge
approved these changes
Nov 10, 2020
Closed
rabbbit
pushed a commit
that referenced
this pull request
Jun 6, 2022
Bobjohnson original version seems to be more used (again). Also see history in #39.
storozhukBM
added a commit
to storozhukBM/ratelimit
that referenced
this pull request
Jun 6, 2022
Bobjohnson original version seems to be more used (again). Also see history in uber-go#39.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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:
if they're scheduled at the "now" boundry
specific time (not a problem for us, so not touching it)
By dropping afterFunc into Sleep we're able to deprecate the internal
fork altogether - there is a missing nap in andres-erbsen/clock.AfterFunc
that meant we needed our own fork. Now it's not needed.