[refine](code) remove dead runtime helpers and deprecated MaxCompute fields#63026
[refine](code) remove dead runtime helpers and deprecated MaxCompute fields#63026Mryange wants to merge 1 commit intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Requesting changes for one compatibility regression in the BE cleanup.
Critical checkpoint conclusions:
- Goal/test proof: the PR aims to remove dead code; most removed accessors/helpers appear unused, but the MaxCompute deprecated-field fallback is still part of the FE-BE thrift compatibility surface and is not covered by tests here.
- Scope/focus: generally focused on BE cleanup, except it removes behavior needed for old
TMCTablepayloads. - Concurrency: no concurrency changes found in the actual GitHub PR diff.
- Lifecycle/static init: no new static initialization or lifecycle issue found.
- Config/protocol compatibility: issue found; removing deprecated MaxCompute credential fallback can break mixed-version or old serialized descriptors.
- Parallel paths: no missing parallel code path found in the reviewed diff.
- Testing: no test coverage was added for MaxCompute descriptor compatibility.
- Observability/performance/transaction/data-write concerns: not materially involved by the cleanup.
- User focus: no additional user-provided focus points were supplied.
5dc8594 to
b82ab83
Compare
|
/review |
There was a problem hiding this comment.
Review summary:
I reviewed the current GitHub PR diff and the existing inline review context. I did not find additional issues in the currently reviewable BE runtime cleanup files (, , , , and related removed empty translation units). I am not duplicating the already-open MaxCompute descriptor compatibility thread.
Critical checkpoint conclusions:
- Goal and tests: The current reviewable diff is a dead-code/runtime-helper cleanup. The removed helpers appear unused in the current tree; no new functional test is required for those removals, but the existing MaxCompute compatibility concern remains tracked separately.
- Scope/focus: The reviewable changes are small and focused on unused BE helpers/empty source files.
- Concurrency/lifecycle: No new concurrency or non-trivial lifecycle management is introduced in the current reviewable diff.
- Config/compatibility/protocol: No new config or protocol changes are present in the current reviewable diff. Compatibility concern for MaxCompute deprecated fields is already raised in the existing thread.
- Parallel paths/data correctness/memory safety: No additional data correctness, memory tracking, or parallel-path issues found in the reviewed BE cleanup files.
- Observability/performance: No new observability requirement or performance concern found.
User focus: No additional user-provided review focus was supplied.
There was a problem hiding this comment.
Review summary:
I reviewed the current GitHub PR diff and the existing inline review context. I did not find additional issues in the currently reviewable BE runtime cleanup files (GenericReader, CollectionValue, ResultQueueMgr, RuntimeState, and related removed empty translation units). I am not duplicating the already-open MaxCompute descriptor compatibility thread.
Critical checkpoint conclusions:
- Goal and tests: The current reviewable diff is a dead-code/runtime-helper cleanup. The removed helpers appear unused in the current tree; no new functional test is required for those removals, but the existing MaxCompute compatibility concern remains tracked separately.
- Scope/focus: The reviewable changes are small and focused on unused BE helpers/empty source files.
- Concurrency/lifecycle: No new concurrency or non-trivial lifecycle management is introduced in the current reviewable diff.
- Config/compatibility/protocol: No new config or protocol changes are present in the current reviewable diff. Compatibility concern for MaxCompute deprecated fields is already raised in the existing thread.
- Parallel paths/data correctness/memory safety: No additional data correctness, memory tracking, or parallel-path issues found in the reviewed BE cleanup files.
- Observability/performance: No new observability requirement or performance concern found.
User focus: No additional user-provided review focus was supplied.
|
run buildall |
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
This commit cleans up dead BE-side runtime and scan code that remained after earlier refactors.
GenericReader no longer has any out-of-line logic, several CollectionValue, ResultQueueMgr, RuntimeState, and descriptor helpers have no remaining callers, and MaxComputeTableDescriptor still cached deprecated TMCTable fields and rebuilt authentication properties from them even though FE already sends authentication settings through TMCTable.properties.
Root cause: deprecated compatibility code and empty translation units were left behind after the MaxCompute path moved to the properties-based contract and after reader/runtime logic was moved or inlined elsewhere. This change removes the empty source files, drops the unused helpers and accessors, and keeps MaxComputeTableDescriptor focused on the fields that are still consumed at runtime.
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)