-
-
Notifications
You must be signed in to change notification settings - Fork 735
Add rate limiter exercise #3008
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing this exercise! As a concept for a practice exercise, I think this looks good! Some comments below:
exercises/practice/rate-limiter/src/main/java/FixedWindowRateLimiter.java
Outdated
Show resolved
Hide resolved
exercises/practice/rate-limiter/src/main/java/FixedWindowRateLimiter.java
Outdated
Show resolved
Hide resolved
public void advanceNanos(long nanos) { | ||
this.now = this.now.plusNanos(nanos); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might have forgotten to update the tests to use the advance(Duration d)
method.
final class ClientId { | ||
final String id; | ||
|
||
ClientId(String id) { | ||
this.id = id; | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
return (o instanceof ClientId) && ((ClientId) o).id.equals(this.id); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return id.hashCode(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'm not a fan of having to rely on the correct implementation of the equals
and hashCode
in the test and I think having the UUID
, Integer
, Long
and String
variants is more than enough to test the generics side - perhaps we could remove this test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback — could you share a bit more on what feels uncomfortable about the custom‑object test?
Is it that it couples outcomes to equals/hashCode correctness and can cause confusing failures unrelated to the rate‑limiter logic?
I have removed this custom object test case to reduce possible confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is that we'd be relying on the equals
and hashCode
being implemented correctly for this test to work properly. If this had been for an actual project, I'd be wondering if we need tests to prove that they have been implemented correctly and honor the equals
and hashCode
contract too. I think an alternative would be to use records which would take care of this for you, but I also felt it was already sufficient to use UUID objects.
exercises/practice/rate-limiter/src/test/java/RateLimiterTest.java
Outdated
Show resolved
Hide resolved
config.json
Outdated
"uuid": "b4b0c60e-4ce1-488e-948f-bcb6821c773c", | ||
"practices": [], | ||
"prerequisites": [], | ||
"difficulty": 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the difficulty, would you say it is similar in difficulty as Luhn? If so, we could say it is a 4 (a 4 is one of the lower medium difficulty, 3 is easy). I think 1 is too low.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think low‑medium (4) is reasonable! it only requires per‑key state and fixed‑window boundary handling with java.time, but avoids concurrency and advanced algorithms like sliding windows or token buckets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! The exercises are sorted by difficulty and then name though. Could you please move this entry to between proverb
and rotational-cipher
in the exercises list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shifted! Thanks!
…va into add-rate-limiter-exercise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its almost there! I have noticed the CI is failing with some errors (see the results at the bottom). I think one of the errors may be because there are some Gradle files missing. To fix this:
- Have a look at resources/exercise-template.
- There are some Gradle files and directories that should be copied to your
rate-limiter
exercise, namely:- the
gradle
directory gradlew
gradlew.bat
- the
@Test | ||
void allowsUpToLimit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've started putting @DisplayName
tags in the tests in the other PRs (example in suggestion). Could you please do the same?
@Test | |
void allowsUpToLimit() { | |
@Test | |
@DisplayName("Allows up to window limit") | |
void allowsUpToLimit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with the tests with @DisplayName
…va into add-rate-limiter-exercise
Co-authored-by: Kah Goh <[email protected]>
Co-authored-by: Kah Goh <[email protected]>
…va into add-rate-limiter-exercise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, there's still a couple of errors from the CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it is passing the CI now! I noticed some more really minor things while doing a last pass over the changes.
exercises/practice/rate-limiter/src/test/java/RateLimiterTest.java
Outdated
Show resolved
Hide resolved
exercises/practice/rate-limiter/src/test/java/RateLimiterTest.java
Outdated
Show resolved
Hide resolved
exercises/practice/rate-limiter/src/test/java/RateLimiterTest.java
Outdated
Show resolved
Hide resolved
…java Co-authored-by: Kah Goh <[email protected]>
…java Co-authored-by: Kah Goh <[email protected]>
…java Co-authored-by: Kah Goh <[email protected]>
Co-authored-by: Kah Goh <[email protected]>
Appreciate the thorough review! |
Co-authored-by: Kah Goh <[email protected]>
Thanks for sticking with this one! Looks good now! |
pull request
Closes #3007
Add Practice Exercise: Rate Limiter
Reviewer Resources:
Track Policies