Skip to content
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.

Sign up for GitHub

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

Jump to bottom

FEAT: Producer side rate limiting #163

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

FEAT: Producer side rate limiting #163

wants to merge 33 commits into from

Conversation

iosdev747
Copy link
Collaborator

No description provided.

@gaurav-ashok
Copy link
Contributor

gaurav-ashok commented Sep 17, 2024 •
edited
Loading

I think code can be placed more appropriately different modules.
Have we thought, why the current current mostly is hosted in entities & server module?

I am hoping for the RL module to be independent. If not completely, then atleast core RL. Can it be done?

gaurav-ashok
gaurav-ashok reviewed Sep 17, 2024
View reviewed changes
server/src/main/java/com/flipkart/varadhi/utils/weights/WeightFunction.java Outdated
package com.flipkart.varadhi.utils.weights;

public interface WeightFunction {
float applyWeight(long time, long currentTime, long windowSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

why this specific interface.
add the documentation.

gaurav-ashok
gaurav-ashok reviewed Sep 30, 2024
View reviewed changes
server/src/main/java/com/flipkart/varadhi/qos/TopicRateLimiter.java Outdated
if(suppressionFactor == 0) {
return true;
}
return currentObserved.get() <= lastObserved.get()*(1-suppressionFactor);
Copy link
Contributor

@gaurav-ashok gaurav-ashok Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why this formula?
i would have expected isAllowed = !(random.nextDouble(0, 1) < spuppressionFactor).

Copy link
Contributor

@gaurav-ashok gaurav-ashok Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The current formula will block everything once your condition becomes true. I am also not sure why lastObserved data point is being used here!

gaurav-ashok
gaurav-ashok reviewed Sep 30, 2024
View reviewed changes
Copy link
Contributor

@gaurav-ashok gaurav-ashok left a comment

Choose a reason for hiding this comment

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

you need to mention on the class about the thread safety requirements. Right now the code is not correct, functionaly. There are bugs, both logic and concurrency related bugs.

server/src/main/java/com/flipkart/varadhi/qos/TopicRateLimiter.java Outdated
if(suppressionFactor == 0) {
return true;
}
return currentObserved.get() <= lastObserved.get()*(1-suppressionFactor);
Copy link
Contributor

@gaurav-ashok gaurav-ashok Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The current formula will block everything once your condition becomes true. I am also not sure why lastObserved data point is being used here!

server/src/main/java/com/flipkart/varadhi/qos/TopicRateLimiter.java Outdated Show resolved Hide resolved
server/src/main/java/com/flipkart/varadhi/qos/entity/SuppressionFactor.java Outdated Show resolved Hide resolved
server/src/main/java/com/flipkart/varadhi/qos/entity/SuppressionData.java Outdated Show resolved Hide resolved
server/src/main/java/com/flipkart/varadhi/qos/entity/TrafficData.java Outdated Show resolved Hide resolved
server/src/main/java/com/flipkart/varadhi/verticles/webserver/TrafficAggregator.java Outdated
}

public void sendUsageToController() {
scheduledExecutorService.scheduleAtFixedRate(() -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

double check the interplay between "scheduleAtFixedRate" and the delay in getting the suppression data.

server/src/main/java/com/flipkart/varadhi/verticles/webserver/TrafficAggregator.java Outdated Show resolved Hide resolved
server/src/main/java/com/flipkart/varadhi/verticles/webserver/TrafficAggregator.java Outdated Show resolved Hide resolved
server/src/main/java/com/flipkart/varadhi/verticles/webserver/TrafficAggregator.java Outdated Show resolved Hide resolved
server/src/main/java/com/flipkart/varadhi/verticles/webserver/TrafficAggregator.java Outdated Show resolved Hide resolved
iosdev747
iosdev747 commented Oct 5, 2024
View reviewed changes
Copy link
Collaborator Author

@iosdev747 iosdev747 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

TODO:

  • Move RL service out of server module.

@anuj-flipkart anuj-flipkart linked an issue Oct 24, 2024 that may be closed by this pull request
FEAT: Producer side rate limiting #173
Open
@codecov-commenter
Copy link

codecov-commenter commented Nov 6, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 63.94984% with 115 lines in your changes missing coverage. Please review.

Project coverage is 70.74%. Comparing base (78be60c) to head (03c11fc).
Report is 36 commits behind head on master.

Files with missing lines Patch % Lines
...varadhi/controller/DistributedRateLimiterImpl.java 31.42% 24 Missing ⚠️
.../com/flipkart/varadhi/qos/TopicMetricsHandler.java 0.00% 21 Missing ⚠️
...a/com/flipkart/varadhi/qos/RateLimiterMetrics.java 40.00% 15 Missing ⚠️
...va/com/flipkart/varadhi/cluster/MessageRouter.java 10.00% 9 Missing ⚠️
...in/java/com/flipkart/varadhi/utils/FutureUtil.java 0.00% 8 Missing ⚠️
...varadhi/qos/weights/ExponentialWeightFunction.java 0.00% 6 Missing ⚠️
.../com/flipkart/varadhi/cluster/MessageExchange.java 14.28% 6 Missing ⚠️
...com/flipkart/varadhi/qos/entity/ClientHistory.java 83.33% 1 Missing and 4 partials ⚠️
...rt/varadhi/qos/weights/ConstantWeightFunction.java 0.00% 4 Missing ⚠️
...kart/varadhi/qos/weights/LinearWeightFunction.java 0.00% 4 Missing ⚠️
... and 6 more
Additional details and impacted files 
@@             Coverage Diff              @@
##             master     #163      +/-   ##
============================================
+ Coverage     63.11%   70.74%   +7.62%     
- Complexity      543     1171     +628     
============================================
  Files           133      217      +84     
  Lines          2790     5425    +2635     
  Branches        168      330     +162     
============================================
+ Hits           1761     3838    +2077     
- Misses          964     1430     +466     
- Partials         65      157      +92

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

gauravAshok
gauravAshok reviewed Nov 7, 2024
View reviewed changes
ratelimiter/src/main/java/com/flipkart/varadhi/qos/DistributedRateLimiter.java Outdated
* DistributedRateLimiter interface that takes in loadInfo from all the clients and returns the SuppressionData for each
* topic so that FactorRateLimiter can use it to limit the traffic.
*/
public interface DistributedRateLimiter extends RateLimiter<SuppressionData, ClientLoadInfo> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

yup this works :)

gauravAshok
gauravAshok reviewed Nov 7, 2024
View reviewed changes
ratelimiter/src/main/java/com/flipkart/varadhi/qos/entity/ClientHistory.java Outdated

@Slf4j
public class ClientHistory {
Map<String, Deque<TopicLoadInfo>> clientHistoryMap; // clientId to history records
Copy link
Collaborator

Choose a reason for hiding this comment

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

cant we use array / arraylist instead of dequeu?

gauravAshok
gauravAshok reviewed Nov 7, 2024
View reviewed changes
settings.gradle Outdated Show resolved Hide resolved
gauravAshok
gauravAshok requested changes Nov 7, 2024
View reviewed changes
Copy link
Collaborator

@gauravAshok gauravAshok left a comment

Choose a reason for hiding this comment

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

Some more minor comments

@gauravAshok gauravAshok marked this pull request as ready for review November 12, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gauravAshok gauravAshok gauravAshok requested changes

@gaurav-ashok gaurav-ashok gaurav-ashok left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FEAT: Producer side rate limiting
4 participants
@iosdev747 @gaurav-ashok @codecov-commenter @gauravAshok