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

fix: rate limiting should be ineffective when RateLimitInfo is not present #2243

Merged
merged 5 commits into from
May 30, 2024
Merged

fix: rate limiting should be ineffective when RateLimitInfo is not present #2243

merged 5 commits into from
May 30, 2024

Conversation

kongweihan
Copy link
Contributor

@kongweihan kongweihan commented May 22, 2024 •
edited by igorbernstein2
Loading

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)
  • Rollback plan is reviewed and LGTMed
  • All new data plane features have a completed end to end testing plan

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: bigtable Issues related to the googleapis/java-bigtable API. labels May 22, 2024
@kongweihan kongweihan force-pushed the fix-flow-control-rate-limit-info-missing branch 2 times, most recently from 4f0fa05 to 08881af Compare May 29, 2024 16:18
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels May 29, 2024
@kongweihan kongweihan marked this pull request as ready for review May 29, 2024 16:19
@kongweihan kongweihan requested review from a team as code owners May 29, 2024 16:19
@kongweihan kongweihan requested a review from igorbernstein2 May 29, 2024 16:20
igorbernstein2
igorbernstein2 requested changes May 29, 2024
View reviewed changes
...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java Show resolved Hide resolved
...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java Outdated Show resolved Hide resolved
...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java Outdated Show resolved Hide resolved
...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java Outdated Show resolved Hide resolved
...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java Outdated Show resolved Hide resolved
...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java Outdated Show resolved Hide resolved
...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java Outdated Show resolved Hide resolved
...rc/main/java/com/google/cloud/bigtable/data/v2/stub/RateLimitingServerStreamingCallable.java Show resolved Hide resolved
@kongweihan kongweihan force-pushed the fix-flow-control-rate-limit-info-missing branch 3 times, most recently from b919180 to e02261f Compare May 29, 2024 20:02
@kongweihan
Copy link
Contributor Author

@igorbernstein2 what kind of rollback plan we could implement for such changes?

@kongweihan kongweihan force-pushed the fix-flow-control-rate-limit-info-missing branch 2 times, most recently from 94fd611 to da5fb62 Compare May 30, 2024 15:27
@kongweihan
Rate limiting should be ineffective when RateLimitInfo is not present
b219948 
Built a new ConditionalRateLimiter that can be disabled but keep the
rate.

Also moved setRate() into the ConditionalRateLimiter. Update and disable
is throttled based on period from RateLimitInfo.

By default the rate limiter is disabled when initiated.

Enable, disable and update rate will be logged as INFO, as they're
critical to the run of the worker and only logged per 10s by default for
each worker thread.
@kongweihan kongweihan force-pushed the fix-flow-control-rate-limit-info-missing branch from da5fb62 to b219948 Compare May 30, 2024 15:33
@igorbernstein2
Copy link
Contributor

re rollback - this is the rollback mechanism for the throttling feature. I dont think we need a rollback mechanism for the the rollback mechanism :)

@igorbernstein2 igorbernstein2 changed the title Rate limiting should be ineffective when RateLimitInfo is not present fix: rate limiting should be ineffective when RateLimitInfo is not present May 30, 2024
@conventional-commit-lint-gcf Conventional Commit Lint GCF
Copy link

conventional-commit-lint-gcf bot commented May 30, 2024 •
edited
Loading

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@igorbernstein2 igorbernstein2 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 30, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 30, 2024
@kongweihan
fix: Rate limiting should be disabled when RateLimitInfo is not valid
ce64f7e 
Built a new ConditionalRateLimiter that can be disabled but keep the
rate.

Also moved setRate() into the ConditionalRateLimiter. Update and disable
is throttled based on period from RateLimitInfo.

By default the rate limiter is disabled when initiated.

Enable, disable and update rate will be logged as INFO, as they're
critical to the run of the worker and only logged per 10s by default for
each worker thread.
@kongweihan kongweihan force-pushed the fix-flow-control-rate-limit-info-missing branch from b219948 to ce64f7e Compare May 30, 2024 20:51
@igorbernstein2 igorbernstein2 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 30, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 30, 2024
@gcf-owl-bot gcf-owl-bot bot requested a review from a team as a code owner May 30, 2024 20:52
@igorbernstein2 igorbernstein2 added the automerge Merge the pull request once unit tests and other checks pass. label May 30, 2024
@gcf-merge-on-green gcf-merge-on-green bot merged commit a0ec901 into googleapis:main May 30, 2024
20 of 21 checks passed
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label May 30, 2024
@release-please release-please bot mentioned this pull request May 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igorbernstein2 igorbernstein2 igorbernstein2 approved these changes

Assignees
No one assigned
Labels
api: bigtable Issues related to the googleapis/java-bigtable API. size: l Pull request size is large.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kongweihan @igorbernstein2