Skip to content

Preserve Redis line scan progress across reads - #3666

Open
wasphin wants to merge 1 commit into
apache:masterfrom
wasphin:fix-redis-line-scan-progress
Open

wasphin wants to merge 1 commit into
apache:masterfrom
wasphin:fix-redis-line-scan-progress

Conversation

@wasphin

@wasphin wasphin commented Oct 25, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Problem Summary:

Inline Redis commands and simple-string replies restart their CRLF search
from the beginning whenever more input arrives. Small fragments repeatedly
scan the same bytes and backing blocks.

What is changed and the side effects?

Changed:

  • Preserve the current backing block, offset and CR state between reads.
  • Bound the reply scan by the existing allocation limit.
  • Reuse inactive reply-union storage for scan state, preserving reply size.
  • Cover fragmented commands, status/error replies, nested arrays, CRLF
    crossing backing blocks, and scan limits.

Side effects:

  • Performance effects: Previously scanned bytes and blocks are skipped on
    later reads. Each command parser stores 16 bytes of scan state.
  • Breaking backward compatibility: No API signature changes. Existing input
    bytes must remain unchanged while the parser waits for more data.

Check List:

@chenBright
chenBright requested a balanced review from Copilot October 26, 2026 03:07

This comment was marked as low quality.

Copilot AI left a comment

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.

🟡 Changes recommended

Partial reply copying loses scan progress, and incomplete simple replies can report an invalid size.

1 open issue

Review effort: Lite

Comment thread src/brpc/redis_reply.cpp
Comment on lines +153 to +156
if (_length != kScanningLine) {
_type = REDIS_REPLY_NIL;
_length = kScanningLine;
_data.line = {};
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.

2 participants