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

feat(publisher): add clear rate limit for tiup publishing #213

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

wuhuizuo
Copy link
Contributor

Signed-off-by: wuhuizuo [email protected]

@ti-chi-bot ti-chi-bot bot requested a review from purelind December 17, 2024 04:16
Copy link

ti-chi-bot bot commented Dec 17, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

The pull request title indicates that the changes found within are related to the addition of a clear rate limit for tiup publishing.

Here are the key changes in the pull request:

  1. A new endpoint reset-rate-limit is added to the service "tiup". This endpoint allows for rate limit reset functionality.
  2. The new endpoint is registered in the client and server files for the "tiup" service.
  3. A new handler for the reset-rate-limit endpoint is added.
  4. A new function ResetRateLimit is added to the tiup service interface and its implementation.
  5. A rate limit key is added in the rate limit check function.
  6. The openAPI files are updated to include the new endpoint.

Potential problems:

  1. There is no clear indication of what the rate limit actually is. If the rate limit is too low, it may hinder usage of the service. If it's too high, it may not protect the service effectively.
  2. There is no authentication or authorization in place for the rate limit reset endpoint. This could lead to abuse of the system.
  3. The rate limit resetting is done by deleting keys from Redis. This could potentially cause issues if other data is being stored in Redis under similar keys.
  4. There are no tests included with these changes. Without tests, it's hard to ensure that the new functionality is working as expected.

Suggestions to fix the problems:

  1. The rate limit should be clearly defined somewhere, either in the code or in documentation.
  2. Implement some form of authentication or authorization for the reset-rate-limit endpoint to prevent unauthorized access.
  3. Consider namespacing or otherwise differentiating the keys used for rate limiting to avoid conflicts.
  4. Add tests to cover the new functionality and any related functions to ensure everything is working as expected.

@ti-chi-bot ti-chi-bot bot added the size/L label Dec 17, 2024
@wuhuizuo
Copy link
Contributor Author

/approve

Copy link

ti-chi-bot bot commented Dec 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Dec 17, 2024
@ti-chi-bot ti-chi-bot bot merged commit 020157a into main Dec 17, 2024
7 checks passed
@ti-chi-bot ti-chi-bot bot deleted the feature/clear-rate-limit branch December 17, 2024 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant