Skip to content

ruv2: align bypass with ru v1#1926

Merged
ti-chi-bot[bot] merged 3 commits intotikv:masterfrom
disksing:disksing/ruv2-bypass-upstream
Mar 30, 2026
Merged

ruv2: align bypass with ru v1#1926
ti-chi-bot[bot] merged 3 commits intotikv:masterfrom
disksing:disksing/ruv2-bypass-upstream

Conversation

@disksing
Copy link
Copy Markdown
Collaborator

@disksing disksing commented Mar 30, 2026

Summary

  • bypass TiKV RU v2 updates only for requests that are already marked as bypassed by RequestInfo
  • reuse the same bypass decision in unary, async, and coprocessor stream response paths
  • add regression coverage for async requests, coprocessor streams, and the bypass decision itself

Why

RU v2 bypass conditions had become broader than the existing resource control bypass rules by also skipping background requests. This change aligns the RU v2 bypass behavior with the existing request-info bypass semantics.

Testing

  • go test ./internal/client -run 'TestBypassRUV2FollowsRequestInfoBypass|TestSendRequestAsyncUpdateTiKVRUV2'
  • go test ./tikvrpc -run TestCopStreamResponseRecvBypassRUV2
  • go test ./internal/resourcecontrol -run TestMakeRequestInfo

Summary by CodeRabbit

  • Bug Fixes

    • Resource-usage V2 updates are now conditionally applied per request, preventing unintended updates for requests marked to bypass tracking.
    • Coprocessor streaming responses now honor the per-request bypass flag, avoiding resource-usage updates when bypass is set.
  • Tests

    • Added tests covering bypass behavior and request-path handling to ensure correct tracking and non-tracking scenarios.

Signed-off-by: disksing <i@disksing.com>
(cherry picked from commit 26d927f)
@ti-chi-bot ti-chi-bot bot added dco-signoff: yes Indicates the PR's author has signed the dco. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 30, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: da455d35-dc13-4a25-8f8e-986da54e94b8

📥 Commits

Reviewing files that changed from the base of the PR and between bc89af0 and f6a534a.

📒 Files selected for processing (3)
  • internal/client/client.go
  • tikvrpc/tikvrpc.go
  • tikvrpc/tikvrpc_test.go
✅ Files skipped from review due to trivial changes (1)
  • tikvrpc/tikvrpc_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • tikvrpc/tikvrpc.go
  • internal/client/client.go

📝 Walkthrough

Walkthrough

Adds a request-derived Bypass flag (from resourcecontrol.MakeRequestInfo(req).Bypass()) to gate TiKV RU v2 updates across sync, async, and coprocessor-stream flows; coprocessor stream responses carry the flag and tests cover bypass and non-bypass behaviors.

Changes

Cohort / File(s) Summary
Synchronous & Asynchronous Client Handling
internal/client/client.go, internal/client/client_async.go
Derive per-request Bypass via resourcecontrol.MakeRequestInfo(req).Bypass() and skip config.UpdateTiKVRUV2FromExecDetailsV2(...) when bypass is true; async path also requires resp != nil before updating.
Coprocessor Stream Response
tikvrpc/tikvrpc.go
Added Bypass bool field to CopStreamResponse and make Recv() conditional on !resp.Bypass before invoking RU v2 update.
Async Request Tests
internal/client/client_async_test.go
Added a bypass-path verification in TestSendRequestAsyncUpdateTiKVRUV2 that injects RUDetails and a RequestSource = "xxx_internal_others" request and asserts RU v2 is not updated.
Request Info Bypass Unit Test
internal/client/client_interceptor_test.go
Added TestBypassRUV2FollowsRequestInfoBypass to assert resourcecontrol.MakeRequestInfo(req).Bypass() true for internal request source and false for default/background/resource-group cases.
Coprocessor Stream Tests
tikvrpc/tikvrpc_test.go
Added TestCopStreamResponseRecvBypass with a mock stream client to verify Recv() updates RU v2 when Bypass is false and skips update when Bypass is true.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant ResourceControl
    participant TiKV_RPC as tikvrpc
    participant Config

    Client->>ResourceControl: MakeRequestInfo(req).Bypass()
    ResourceControl-->>Client: bypass (true/false)
    Client->>TiKV_RPC: send request (include Bypass for streams)
    alt Non-stream / response arrives
        TiKV_RPC-->>Client: resp (may be nil)
        alt resp != nil and bypass == false
            Client->>Config: UpdateTiKVRUV2FromExecDetailsV2(ctx, execDetails, 0)
        else
            Note right of Client: Skip RU v2 update
        end
    else Coprocessor stream
        TiKV_RPC->>TiKV_RPC: CopStreamResponse.Bypass = bypass
        loop Recv messages
            TiKV_RPC-->>Config: if !Bypass then UpdateTiKVRUV2FromExecDetailsV2(ctx, execDetails, 0)
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

lgtm, size/M

Suggested reviewers

  • nolouch
  • ekexium
  • JmPotato

Poem

🐰 A tiny flag hops through the call,
Skipping the metrics for requests small,
Streams carry the token along the way,
Tests nod and let the normal flows play,
Hooray — RU v2 minding, clean and small.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'ruv2: align bypass with ru v1' directly summarizes the main change: aligning RU v2 bypass behavior with the existing RU v1 bypass semantics via RequestInfo.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@disksing disksing changed the title ruv2: align bypass with request info ruv2: align bypass with ru v1 Mar 30, 2026
@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Mar 30, 2026
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
@ti-chi-bot ti-chi-bot bot added the lgtm label Mar 30, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot bot commented Mar 30, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ekexium, nolouch

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

The pull request process is described here

Details 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 approved and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Mar 30, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot bot commented Mar 30, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-03-30 04:12:35.471815504 +0000 UTC m=+151960.677175561: ☑️ agreed by nolouch.
  • 2026-03-30 06:05:14.872378996 +0000 UTC m=+158720.077739103: ☑️ agreed by ekexium.

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

Labels

approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants