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.

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

ratelimit: support modify BBR config by API #7291

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

Conversation

CabinfeverB
Copy link
Member

@CabinfeverB CabinfeverB commented Oct 31, 2023

What problem does this PR solve?

Issue Number: ref #7167

What is changed and how does it work?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Code changes

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

Release note

None.

Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
@@ -0,0 +1,87 @@
// Copyright 2022 TiKV Project Authors.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2022 TiKV Project Authors.
// Copyright 2023 TiKV Project Authors.

Comment on lines 71 to 72
l.mu.RLock()
defer l.mu.RUnlock()
Copy link
Member

Choose a reason for hiding this comment

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

Rlock is enough?

h.rd.JSON(w, http.StatusOK, result)
}
}
}

func getQPSSetting(input map[string]interface{}, cfg *ratelimit.DimensionConfig) bool {
qps, ok := input["qps"].(float64)
Copy link
Member

Choose a reason for hiding this comment

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

How about moving it to the caller or merging the getxxxSetting?

return func(label string, l *Limiter) UpdateStatus {
// UpdateDimensionConfigForTest creates QPS limiter and concurrency limiter for a given label by config if it doesn't exist.
// only used in test.
func UpdateDimensionConfigForTest(cfg *DimensionConfig, opt ...bbrOption) Option {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any difference with line 77? Can we just use this one?

Signed-off-by: Cabinfever_B <[email protected]>

address comment

Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 5, 2024
Copy link
Contributor

ti-chi-bot bot commented Jan 5, 2024

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
@CabinfeverB CabinfeverB marked this pull request as draft February 20, 2024 06:10
@ti-chi-bot ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2024
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Signed-off-by: Cabinfever_B <[email protected]>
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

Merging #7291 (34b6821) into master (1772ad0) will increase coverage by 0.07%.
Report is 85 commits behind head on master.
The diff coverage is 96.31%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7291      +/-   ##
==========================================
+ Coverage   73.45%   73.53%   +0.07%     
==========================================
  Files         432      433       +1     
  Lines       47886    48123     +237     
==========================================
+ Hits        35175    35387     +212     
- Misses       9667     9683      +16     
- Partials     3044     3053       +9     
Flag Coverage Δ
unittests 73.53% <96.31%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@CabinfeverB CabinfeverB removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2024
@CabinfeverB CabinfeverB marked this pull request as ready for review February 22, 2024 16:46
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 22, 2024
@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 9, 2024
Copy link
Contributor

ti-chi-bot bot commented Apr 9, 2024

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants