Skip to content

Fix: Remove recursion of redis command parser#3136

Merged
wwbmmm merged 1 commit intoapache:masterfrom
howzi:remove_recursion_of_redis_command_parser
Nov 15, 2025
Merged

Fix: Remove recursion of redis command parser#3136
wwbmmm merged 1 commit intoapache:masterfrom
howzi:remove_recursion_of_redis_command_parser

Conversation

@howzi
Copy link
Copy Markdown
Contributor

@howzi howzi commented Nov 3, 2025

What problem does this PR solve?

Issue Number: NULL

Problem Summary: Remove recursion of redis command parser

What is changed and the side effects?

Changed: Convert RedisCommandParser::Consume to iterative state machine (remove recursion)

Side effects:

  • Performance effects:

  • Breaking backward compatibility:


Check List:

@howzi howzi changed the title Remove recursion of redis command parser Fix: Remove recursion of redis command parser Nov 3, 2025
@howzi
Copy link
Copy Markdown
Contributor Author

howzi commented Nov 3, 2025

@chenBright take a look plz, same problem as #3099. Although MAX_ONCE_READ is 512k, but is enough for a 1024k args redis command(the new UT).

if (++_index == _length) {
break;
}
} while (true);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This do-while is too long, it's hard to read. I think you can move the actual parsing code to a ConsumeImpl function, and leave this do-while structure in the Consume function.

@howzi howzi force-pushed the remove_recursion_of_redis_command_parser branch from 571ead7 to 53587e4 Compare November 4, 2025 13:00
@wwbmmm
Copy link
Copy Markdown
Contributor

wwbmmm commented Nov 5, 2025

LGTM

Copy link
Copy Markdown
Contributor

@chenBright chenBright left a comment

Choose a reason for hiding this comment

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

LGTM

@howzi howzi requested a review from wwbmmm November 6, 2025 02:16
@howzi
Copy link
Copy Markdown
Contributor Author

howzi commented Nov 14, 2025

@wwbmmm @chenBright Is this available to merge?

@wwbmmm wwbmmm merged commit 645013b into apache:master Nov 15, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants