[fix](cloud) Prevent old clients from decoding unknown MetaServiceCode as OK - #64148
Merged
gavinchou merged 4 commits intoAug 1, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
wyxxxcat
force-pushed
the
support_new_ms_old_be_rate_limit
branch
3 times, most recently
from
June 6, 2026 09:09
7a1de92 to
8a2a4cc
Compare
wyxxxcat
force-pushed
the
support_new_ms_old_be_rate_limit
branch
from
June 6, 2026 09:17
8a2a4cc to
af9acb3
Compare
Collaborator
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29319 ms |
Contributor
TPC-DS: Total hot run time: 169488 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Collaborator
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary:
I found one blocking compatibility/backpressure issue. The PR adds the request capability bit and correctly avoids returning the new MS_TOO_BUSY enum to requests that do not advertise support, but the unsupported path currently skips the MS-wide stress rejection entirely instead of downgrading it to a legacy retryable code. That leaves old BEs able to continue full traffic during rolling upgrade exactly when the meta service is trying to shed load.
Critical checkpoints:
- Goal/test: The goal is to gate MS_TOO_BUSY by BE capability. The capability marking and proto fields are present, and helper tests cover reflection/whitelist behavior, but the legacy unsupported-client behavior is not covered and does not match the intended downgrade behavior.
- Scope/focus: The actual PR is small and focused on cloud meta-service rate-limit compatibility.
- Concurrency/lifecycle: No new shared mutable state beyond existing atomics/whitelist lock patterns; no new lifecycle hazard found in the reviewed patch.
- Configuration: No new config item. Existing rate-limit switches are reused.
- Compatibility: Blocking issue below: old clients avoid the unknown enum, but they also bypass MS-wide stress rejection instead of receiving a known retryable status.
- Parallel paths: retry_rpc request types were updated with the capability field; manual get_rowset marks the field too.
- Tests: Added unit tests do not cover an old request without support_ms_too_busy under stress.
- Observability/performance/transactions: No additional issue found beyond the backpressure bypass risk.
User focus: No additional user-provided review focus was specified.
wyxxxcat
force-pushed
the
support_new_ms_old_be_rate_limit
branch
from
July 2, 2026 03:53
af9acb3 to
e9c6a9b
Compare
Collaborator
Author
|
run buildall |
Collaborator
Author
|
run cloudut |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Review summary for e9c6a9b.
I did not submit any new inline comments. The only substantiated issue I found is the legacy-request stress rejection problem already covered by existing inline thread 3379332199 on cloud/src/meta-service/meta_service_helper.h; I treated it as duplicate context rather than posting the same issue again. That thread still appears applicable to the current patch and should be resolved before merge.
Critical checkpoint conclusions:
- Goal/test: The PR tries to gate
MS_TOO_BUSYby request capability. New BE requests sent throughretry_rpcand the hand-rolled GetRowset path are marked, but the legacy no-field behavior described in the PR is already called out by the existing thread. No new test was added; the missing old-client/no-field test is part of that existing feedback. - Scope: The code change is small and limited to BE cloud request marking, meta-service preprocessing, and proto request fields.
- Concurrency/lifecycle: No new shared mutable lifecycle or lock ordering issue found.
- Config/compatibility: No new config was added. This is a rolling-upgrade/proto compatibility change; field numbers were appended, and the remaining compatibility concern is the existing duplicate thread.
- Parallel paths: The generic
retry_rpcpath and the specialsync_tablet_rowsets_unlockedGetRowset path both set the capability marker. The default MS stress whitelist maps to request types that received the new field. - Transaction/persistence/data correctness: No additional transaction, persistence, or visible-version issue found beyond the existing busy-code compatibility concern.
- Performance/observability: No new blocking performance or observability issue found. The added reflection is outside a proven hot enough path for a separate review comment.
- Validation: Verified the current PR head and GitHub changed-file list. The exact GitHub patch reverse-applies cleanly with whitespace errors treated as failures. I did not run a build or tests because this runner lacks
thirdparty/installedandthirdparty/installed/bin/protoc; the checkout is also shallow/grafted, so GitHub PR metadata was used as authoritative scope.
User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite and tests-session-config both returned NO_NEW_VALUABLE_FINDINGS for the final ledger/comment set in convergence round C-001. No subagent candidate became a new inline comment; M-001 was merged as duplicate_of_existing_thread 3379332199.
Contributor
TPC-H: Total hot run time: 29491 ms |
Contributor
TPC-DS: Total hot run time: 180302 ms |
Contributor
ClickBench: Total hot run time: 24.88 s |
wyxxxcat
force-pushed
the
support_new_ms_old_be_rate_limit
branch
2 times, most recently
from
July 9, 2026 14:37
d328f8d to
08f0603
Compare
wyxxxcat
force-pushed
the
support_new_ms_old_be_rate_limit
branch
6 times, most recently
from
August 1, 2026 06:48
47712fd to
13d30c5
Compare
wyxxxcat
force-pushed
the
support_new_ms_old_be_rate_limit
branch
from
August 1, 2026 07:07
13d30c5 to
b8a9fe7
Compare
Collaborator
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29082 ms |
Contributor
TPC-DS: Total hot run time: 168812 ms |
Contributor
ClickBench: Total hot run time: 25.41 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
liaoxin01
changed the title
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
gavinchou
approved these changes
Aug 1, 2026
wyxxxcat
added a commit
to wyxxxcat/doris
that referenced
this pull request
Aug 1, 2026
…e as OK (apache#64148) Problem Summary: Older BE versions may treat unknown meta service enum codes as OK under proto2 semantics. Add a request capability marker so MS only returns MS_TOO_BUSY to BE versions that can handle it, and downgrade otherwise. - Behavior changed: Yes (MS_TOO_BUSY is returned only when the BE request declares support; legacy requests receive KV_TXN_CONFLICT instead)
wyxxxcat
added a commit
to wyxxxcat/doris
that referenced
this pull request
Aug 1, 2026
…e as OK (apache#64148) Problem Summary: Older BE versions may treat unknown meta service enum codes as OK under proto2 semantics. Add a request capability marker so MS only returns MS_TOO_BUSY to BE versions that can handle it, and downgrade otherwise. - Behavior changed: Yes (MS_TOO_BUSY is returned only when the BE request declares support; legacy requests receive KV_TXN_CONFLICT instead)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue Number: close #xxx
Related PR: #xxx
Problem Summary: Older BE versions may treat unknown meta service enum codes as OK under proto2 semantics. Add a request capability marker so MS only returns MS_TOO_BUSY to BE versions that can handle it, and downgrade otherwise.
None
Test: No need to test (not requested; compatibility marker and error-code gating only)
Behavior changed: Yes (MS_TOO_BUSY is returned only when the BE request declares support; legacy requests receive KV_TXN_CONFLICT instead)
Does this need documentation: No
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)