-
Notifications
You must be signed in to change notification settings - Fork 310
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
Feature/bypass filter #194
Conversation
...-jakarta/src/test/java/com/netflix/concurrency/limits/ConcurrencyLimitServletFilterTest.java
Outdated
Show resolved
Hide resolved
...t/java/com/netflix/concurrency/limits/grpc/client/ConcurrencyLimitClientInterceptorTest.java
Outdated
Show resolved
Hide resolved
...-servlet/src/test/java/com/netflix/concurrency/limits/ConcurrencyLimitServletFilterTest.java
Outdated
Show resolved
Hide resolved
...-servlet/src/test/java/com/netflix/concurrency/limits/ConcurrencyLimitServletFilterTest.java
Outdated
Show resolved
Hide resolved
...mits-servlet/src/main/java/com/netflix/concurrency/limits/servlet/ServletLimiterBuilder.java
Show resolved
Hide resolved
...-jakarta/src/test/java/com/netflix/concurrency/limits/ConcurrencyLimitServletFilterTest.java
Show resolved
Hide resolved
...ncy-limits-grpc/src/test/java/com/netflix/concurrency/limits/grpc/util/StringMarshaller.java
Show resolved
Hide resolved
...limits-grpc/src/test/java/com/netflix/concurrency/limits/grpc/util/OptionalResultCaptor.java
Show resolved
Hide resolved
...ore/src/test/java/com/netflix/concurrency/limits/limiter/AbstractPartitionedLimiterTest.java
Show resolved
Hide resolved
...rrency-limits-core/src/main/java/com/netflix/concurrency/limits/limiter/AbstractLimiter.java
Outdated
Show resolved
Hide resolved
*/ | ||
public abstract static class BypassLimiterBuilder<BuilderT extends BypassLimiterBuilder<BuilderT, ContextT>, ContextT> extends Builder<BuilderT> { | ||
|
||
private Predicate<ContextT> bypassResolver = (context) -> false; |
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.
private Predicate<ContextT> bypassResolver = (context) -> false; | |
private static final Predicate<ContextT> ALWAYS_FALSE = (context) -> false; | |
private Predicate<ContextT> bypassResolver = ALWAYS_FALSE; |
* @return Chainable builder | ||
*/ | ||
public BuilderT bypassLimitResolver(Predicate<ContextT> shouldBypass) { | ||
this.bypassResolver = bypassResolver.or(shouldBypass); |
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.
this.bypassResolver = bypassResolver.or(shouldBypass); | |
if(this.bypassResolver == ALWAYS_FALSE) { | |
this.bypassResolver = shouldBypass; | |
} else { | |
this.bypassResolver = bypassResolver.or(shouldBypass); | |
} |
Since this is on the hot path for calls, let's optimize away the default case instead of hoping that the JVM is able to do it later.
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.
This is the builder method. The else condition will be skipped everytime because this.bypassResolver
is aways equal to ALWAYS_FALSE in your suggestion.
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.
Read your code wrong. Applying suggestion.
...rrency-limits-core/src/main/java/com/netflix/concurrency/limits/limiter/AbstractLimiter.java
Outdated
Show resolved
Hide resolved
.bypassLimitResolver(new ShouldBypassPredicate()) | ||
.build(); | ||
|
||
for (int i = 0; i < 1; i++) { |
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.
Nit: this for-loop runs once 😛
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.
It's meant to run once because the partition for batch
calls is 0.1 * 10 = 1
, where 10 is the total limit.
Assert.assertEquals(i+1, limiter.getPartition("live").getInflight()); | ||
Assert.assertTrue(limiter.acquire("admin").isPresent()); | ||
} | ||
} |
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 very clear about the behavior, let's add
} | |
// Verify that bypassed requests are able to proceed even when the limiter is full | |
Assert.assertFalse(limiter.acquire("live").isPresent()); | |
Assert.assertEquals(10, limiter.getInflight()); | |
Assert.assertTrue(limiter.acquire("admin").isPresent()); | |
Assert.assertEquals(10, limiter.getInflight()); | |
} |
I understand that we've effectively covered that above, but this will be easier to understand for someone new to the codebase. It also helps defend against a setup issue (eg: if the limit is increased).
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.
Added extra check. This behavior is also validated for simple limiters in another test.
…mits-core to v0.4.2 (main) (#17049) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.netflix.concurrency-limits:concurrency-limits-core](https://togithub.com/Netflix/concurrency-limits) | `0.4.1` -> `0.4.2` | [![age](https://developer.mend.io/api/mc/badges/age/maven/com.netflix.concurrency-limits:concurrency-limits-core/0.4.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/com.netflix.concurrency-limits:concurrency-limits-core/0.4.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/com.netflix.concurrency-limits:concurrency-limits-core/0.4.1/0.4.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/com.netflix.concurrency-limits:concurrency-limits-core/0.4.1/0.4.2?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>Netflix/concurrency-limits (com.netflix.concurrency-limits:concurrency-limits-core)</summary> ### [`v0.4.2`](https://togithub.com/Netflix/concurrency-limits/releases/tag/v0.4.2) [Compare Source](https://togithub.com/Netflix/concurrency-limits/compare/v0.4.1...v0.4.2) ##### What's Changed - Allow calls to be bypassed without altering limiter state by [@​umairk79](https://togithub.com/umairk79) in [https://github.com/Netflix/concurrency-limits/pull/194](https://togithub.com/Netflix/concurrency-limits/pull/194) **Full Changelog**: Netflix/concurrency-limits@v0.4.1...v0.4.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 10pm every weekday,before 6am every weekday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/camunda/zeebe). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjI2MS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
…mits-core to v0.5.0 (main) (#17163) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.netflix.concurrency-limits:concurrency-limits-core](https://togithub.com/Netflix/concurrency-limits) | `0.4.2` -> `0.5.0` | [![age](https://developer.mend.io/api/mc/badges/age/maven/com.netflix.concurrency-limits:concurrency-limits-core/0.5.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/com.netflix.concurrency-limits:concurrency-limits-core/0.5.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/com.netflix.concurrency-limits:concurrency-limits-core/0.4.2/0.5.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/com.netflix.concurrency-limits:concurrency-limits-core/0.4.2/0.5.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>Netflix/concurrency-limits (com.netflix.concurrency-limits:concurrency-limits-core)</summary> ### [`v0.5.0`](https://togithub.com/Netflix/concurrency-limits/releases/tag/v0.5.0) [Compare Source](https://togithub.com/Netflix/concurrency-limits/compare/v0.4.2...v0.5.0) ##### What's Changed - Add feature to allow calls to be bypassed without altering limiter stater by [@​umairk79](https://togithub.com/umairk79) in [https://github.com/Netflix/concurrency-limits/pull/194](https://togithub.com/Netflix/concurrency-limits/pull/194) and by [@​fedorka](https://togithub.com/fedorka) in [https://github.com/Netflix/concurrency-limits/pull/197](https://togithub.com/Netflix/concurrency-limits/pull/197) - Upgrade nebula oss plugin and gradle to latest version by [@​umairk79](https://togithub.com/umairk79) in [https://github.com/Netflix/concurrency-limits/pull/196](https://togithub.com/Netflix/concurrency-limits/pull/196) **Full Changelog**: Netflix/concurrency-limits@v0.4.1...v0.5.0 ##### Note: the previously released 0.4.2 inadvertently broke binary compatibility in the limiter builders and should not be used. </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 10pm every weekday,before 6am every weekday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/camunda/zeebe). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yNjkuMiIsInVwZGF0ZWRJblZlciI6IjM3LjI2OS4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->
Add capability to bypass calls and not to use limiter algorithm if the provided predicate criteria is met