wyxxxcat opened a new pull request, #70417:
URL: https://github.com/apache/doris/pull/70417

   ### Background
   
   When an FDB commit fails with a transaction conflict, Meta Service can log 
the conflicting keys and the transaction's read and write conflict ranges. 
These diagnostics read FDB special key ranges when enable_logging_conflict_keys 
is enabled.
   
   ### What problem does this PR solve?
   
   Issue Number: None
   Related PR: None
   PR Type: Bugfix
   
   Problem Summary:
   
   Conflict logging can enter an infinite loop when a diagnostic range spans 
multiple pages. The loop repeatedly reads the same completed FDB future and 
appends the same key/value pairs, causing unbounded memory growth and 
potentially an MS OOM. Read all three diagnostic ranges through the existing 
full-range iterator so each page advances on the same transaction. Keep full 
keys for paging and strip the special key prefix only from copied results.
   
   ### Details
   
   - **Frequency:** Triggered when conflict logging is enabled, an FDB commit 
conflicts, and a diagnostic range returns more=true.
   - **Root cause:** Each getter creates one range-read future outside its 
loop. fdb_future_get_keyvalue_array only extracts that future's result; it does 
not fetch the next page. Repeating it leaves more=true and keeps copying the 
same page into the result vector.
   - **Why existing tests missed it:** The existing test uses one conflicting 
key and reads both range ends in one page.
   - **Fix:** Share one helper backed by full_range_get across the 
conflicting-key, read-conflict, and write-conflict getters. Reuse its full-key 
cursor and future ownership, copy each result before advancing, and return 
iterator errors. The original commit error remains unchanged.
   - **Other paths and branches:** All three conflict diagnostic getters use 
the same fix. No backports are included.
   - **Behavior change:** Paginated conflict diagnostics finish reading their 
results or return a read error instead of looping over one page until memory is 
exhausted.
   
   ### Testing
   
   - Extended TxnKvTest.ReportConflictingRange to force one key/value pair per 
page for all three getters and compare the complete results with a single-page 
read. The test also covers an embedded zero byte in the key and a timeout on 
the second page.
   - Static review only. The updated tests have not been run. Compilation and 
formatting checks were not run, as requested by the author.
   
   ### Release note
   
   Fix a possible Meta Service OOM caused by an infinite loop while logging 
paginated FDB conflict ranges with enable_logging_conflict_keys enabled.
   
   ### Check List (For Author)
   
   - Behavior changed: Yes (see Details)
   - Does this need documentation: No
   - Pre-submission status: Partially verified: static review only; 
compilation, formatting checks, and unit tests have not been run.
   
   ### AI Disclosure
   
   - Tool: Codex (GPT-6)
   - Generated: Code, test changes, and this description.
   - Human review: Not confirmed.
   
   ### Background
   
   <!-- How the feature or module this PR touches works today, in two to four 
plain sentences, for a reviewer who doesn't know it. Link the user doc if there 
is one. Leave the problem and the fix to Problem Summary. -->
   
   ### What problem does this PR solve?
   
   <!-- Issue Number: "close #N" if this PR resolves the issue, "#N" if it only 
relates to it, "None" if there is none. A Feature needs an issue. -->
   <!-- Related PR: "#N (relation)" for each PR a reviewer should know about, 
such as "#123 (introduced the bug)", separated by commas. Write "None" if no 
related PR is known. -->
   <!-- PR Type: Bugfix, Performance, Feature, Enhancement, Refactor, Test, 
Build/CI/Chore, Revert, Release/Dependency, or Other. -->
   
   Issue Number:
   Related PR:
   PR Type:
   
   Problem Summary:
   
   <!-- Two to five sentences: the problem or need and who is affected; the 
approach in one sentence; when the diff is larger than that suggests, what else 
it had to handle, or that most of it is tests; and what existing users see 
after an upgrade. -->
   
   ### Details
   
   <!--
   For a diff of more than about 300 changed lines, start with "- **Change 
map:**": how many lines are production code, tests, generated files, and docs, 
then the code in parts, each with the problem it handles and its size.
   Then answer each field of your PR type on its own line, as "- **Field:** 
answer", in this order:
     Bugfix: Frequency, Root cause, Why existing tests missed it, Fix, Other 
paths and branches
     Performance: Why it was slow, Optimization, Results, Applicability and cost
     Feature: Scope, Design, Usage, Default behavior, Compatibility
     Enhancement: Before and after, Configs and defaults, Impact on existing 
users
     Refactor: Moves vs. logic changes, Behavior change, Compatibility, 
Performance impact, Rollback
     Test, adding tests: Scenarios, What existing tests miss, Fails when broken
     Test, fixing a test: Failure, Cause, Stability evidence
     Build/CI/Chore: Affected, Developer and release impact, License and 
third-party, Rollback
     Revert: Why not fix forward, Original author notified, Follow-up
     Release/Dependency: Versions, Upstream changes, Compatibility, Rollback
     Other: Why Other
   "None" is a valid answer when it is true. Don't restate the diff.
   -->
   
   ### Testing
   
   <!-- The tests, suites, or commands that ran, what each shows, and where it 
ran (locally or in CI). For a fix, the test that fails before it and passes 
after. -->
   
   ### Notes for Reviewers
   
   <!-- Optional; delete this section if there is nothing to say. Why each 
non-obvious edit was made, what you are unsure about, and where to start 
reading if Details has no Change map. -->
   
   ### Release note
   
   <!-- One paragraph on what users will notice, or None. -->
   
   ### Check List (For Author)
   
   <!-- Behavior changed: "No", or "Yes (see Details)" with a Behavior change 
field in Details. -->
   <!-- Does this need documentation: "No", or "Yes" with the 
apache/doris-website PR or the plan. -->
   <!-- Pre-submission status: "Ready"; "Partially verified: " and what is not 
verified; or "Not ready: " and why, for a Draft PR. -->
   
   - Behavior changed:
   - Does this need documentation:
   - Pre-submission status:
   
   ### AI Disclosure
   
   <!-- Tool: the agent and model, or None. Generated: the parts it wrote 
(code, tests, this description), or None. Human review: what you reviewed of 
the generated parts, or "Not applicable" without AI tools. You may add "- 
Prompts:" to summarize how you directed the agent. -->
   
   - Tool:
   - Generated:
   - Human review:
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to