re20052 commented on PR #68042: URL: https://github.com/apache/doris/pull/68042#issuecomment-6188474090
> ## Review verdict > Complete static review of PR #68042 at head `10c1aaa440b8e7159d3c920a986142aca4c60f12`. Request changes because the existing [P1 fatal-`Error` propagation finding](https://github.com/apache/doris/pull/68042#discussion_r4024408497) still applies. No distinct new inline finding survived the full review, convergence pass, and final changed-file sweep. > > ## Existing inline threads > * **4024408497 [P1], still applies:** `FutureTask.run()` captures a loader `Error`, while `getRefreshResult()` wraps every `ExecutionException` cause in Nereids `AnalysisException` at lines 133-137. The former direct-owner path caught only `Exception`. The existing thread covers this issue, so its ID is carried instead of reposting it. > * **4024408493 [P1], addressed on this head:** a refresh sets `started` before entering its RPC. A caller arriving after an earlier server snapshot selects a successor and waits for a new RPC after the predecessor finishes. The latch test covers this coordination schedule. > * **4220536704 [P2], addressed for the reported completed-successor case:** after predecessor completion, the successor clears its volatile predecessor reference. If every successor waiter interrupts before starting it, the wrapper can still retain its current prior result until another request; the previous implementation also retained that current result, and this does not establish the reported extra retention after a completed successor. > > ## Critical checkpoint conclusions > 1. **Goal and proof:** the table monitor now handles task admission while Thrift RPC, deserialization, partition rebuilding, and backend-cache inspection run outside it. Four new FE unit tests statically cover successor sharing, ordinary retry, and waiter interruption. They use mocked empty partitions and do not prove real remote partition-delta reconstruction or a live commit. > 2. **Scope and clarity:** the two changed files are focused on refresh coordination and its tests; no unrelated source change was found. > 3. **Concurrency and locks:** concurrent FE planning threads share a table wrapper. The monitor protects `latestRefreshTask` selection; volatile `started` closes admission before the RPC; `FutureTask.run()` executes a callable once; a successor waits for its predecessor before reading and publishing cache hints. Heavy work is outside the table monitor. No distinct lock-order cycle, lost wakeup, overlapping loader, or orphaned successor was substantiated. > 4. **Lifecycle:** the task field is transient, a completed successor clears its predecessor, and an interrupted waiter does not cancel a shared task. A later caller can drive an unstarted successor. No new static-initialization or worker-lifecycle issue was found. > 5. **Configuration:** no configuration item or dynamic-update contract changed. > 6. **Compatibility:** no FE/BE protocol field, function symbol, storage format, persisted metadata format, or rolling-upgrade contract changed. > 7. **Parallel paths:** non-Arrow query binding, remote insert binding, and insert-overwrite planning converge on `getOlapTable()`; Arrow Flight uses a separate scan path. No omitted parallel Thrift refresh path was found. > 8. **Special conditions:** the `isStarted()` admission check and predecessor wait implement the intended generation boundary. The null predecessor is the initial generation; ordinary predecessor failure permits retry. The remaining fatal-cause conversion is the carried P1. > 9. **Test coverage:** the four JUnit tests use latches and bounded waits. They validate the coordination and retry schedules, while real partition/version deltas, backend invalidation, and fatal `Error` propagation remain outside these mocks. The latter is already reported by the existing P1. > 10. **Test results:** no `.out` file changed. Builds and tests were prohibited in this review runner and were not executed; inspected tests and earlier author claims are not independent validation. > 11. **Observability:** ordinary refresh failures still log remote database and table identifiers; no separate logging or metric issue was substantiated. > 12. **Transactions and persistence:** this patch changes metadata-read coordination, not commit/publish, EditLog, replay, or durable state. The remote FE snapshots table and partition metadata under its table read lock. > 13. **Data writes:** remote insert and overwrite planning consume the refreshed table, but no write atomicity or crash-recovery code was modified. No distinct write-path regression was substantiated. > 14. **FE/BE variables:** no new transmitted variable or alternate send path is involved. > 15. **Performance and memory:** expensive metadata work no longer holds the table monitor; successor generations remain serialized and share one result per cohort. The completed predecessor result is released after ordering the successor. No additional hot-path or retained-generation defect was substantiated. > 16. **Other correctness and errors:** ordinary failures remain retryable and successful cache generations are ordered. The still-applicable fatal-`Error` conversion is the sole confirmed blocker; all other investigated risks were dismissed with code evidence or matched existing addressed threads. > > **User focus:** no additional focus point was supplied. Two normal full-review passes and a separate risk-focused pass each returned `NO_NEW_VALUABLE_FINDINGS` in one convergence round; the main-agent final sweep found no unresolved candidate. This review is complete and static-only. > > Existing P0/P1 findings confirmed for this head: [#68042 (comment)](https://github.com/apache/doris/pull/68042#discussion_r4024408497) This finding is an example of over-review. The current code correctly handles recoverable Exceptions. An Error generally indicates that the FE JVM is already in a severely unhealthy state, so special handling of its wrapping or propagation provides little practical value. Moreover, Doris already catches Throwable at higher-level boundaries, so rethrowing the original Error does not establish meaningfully different process-level behavior. This should not be treated as a P1 blocker for this PR. -- 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]
