Skip to content

GCRA param renaming#14950

Merged
minchopaskal merged 2 commits into
redis:unstablefrom
minchopaskal:gcra-rename
Mar 31, 2026
Merged

GCRA param renaming#14950
minchopaskal merged 2 commits into
redis:unstablefrom
minchopaskal:gcra-rename

Conversation

@minchopaskal

@minchopaskal minchopaskal commented Mar 30, 2026

Copy link
Copy Markdown
Collaborator

As per title


Note

Medium Risk
Changes the public GCRA command argument names/tokens, which is backward-incompatible for clients/scripts even though underlying rate-limiting math is unchanged.

Overview
Renames the GCRA command interface to use token terminology: requests-per-period becomes tokens-per-period, and the optional NUM_REQUESTS argument becomes TOKENS (with corresponding error messages/documentation updates).

Updates the implementation variable names and overflow/syntax checks accordingly, and adjusts unit tests and command metadata (commands.def, commands/gcra.json) to match the new argument names.

Written by Cursor Bugbot for commit 1a0eb2c. This will update automatically on new commits. Configure here.

@minchopaskal minchopaskal requested a review from sundb March 30, 2026 08:57
@minchopaskal

Copy link
Copy Markdown
Collaborator Author

@sundb FYI PM wanted this.

@minchopaskal minchopaskal added the state:needs-doc-pr requires a PR to redis-doc repository label Mar 30, 2026
@augmentcode

augmentcode Bot commented Mar 30, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR renames GCRA command parameters to use “tokens” terminology instead of “requests”.

Changes:

  • Renamed the argument requests-per-period to tokens-per-period in command definitions.
  • Renamed the optional keyword NUM_REQUESTS to TOKENS (and updated the corresponding token name).
  • Updated gcraCommand internals and error strings to match the new naming.
  • Updated unit tests to use the new TOKENS keyword.

Technical Notes: The change touches both the generated command table and the JSON command spec used by COMMAND DOCS/redis-cli tooling.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/gcra.c
Comment thread src/gcra.c

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Comment thread src/gcra.c

@sundb sundb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

dont forget to update the top comment

@minchopaskal minchopaskal merged commit ef536f4 into redis:unstable Mar 31, 2026
19 checks passed
@sundb sundb added the release-notes indication that this issue needs to be mentioned in the release notes label Mar 31, 2026
mgravell added a commit to StackExchange/StackExchange.Redis that referenced this pull request Apr 7, 2026
… a new CI build before we can validate this)
michael-grunder added a commit to phpredis/phpredis that referenced this pull request Apr 7, 2026
michael-grunder added a commit to phpredis/phpredis that referenced this pull request Apr 8, 2026
pierluigilenoci pushed a commit to pierluigilenoci/redis that referenced this pull request Apr 16, 2026
 Renames the `GCRA` command interface to use token terminology:
`requests-per-period` becomes `tokens-per-period`, and the optional
`NUM_REQUESTS` argument becomes `TOKENS` (with corresponding error
messages/documentation updates).
mgravell added a commit to StackExchange/StackExchange.Redis that referenced this pull request Apr 21, 2026
* GCRA: reflect name/protocol change from redis/redis#14950 (note: need a new CI build before we can validate this)

* PR number

* CI version

* more CI version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes state:needs-doc-pr requires a PR to redis-doc repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants