Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 17073 ms |
TPC-DS: Total hot run time: 83429 ms |
|
run nonConcurrent |
ClickBench: Total hot run time: 14.91 s |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review of a33291d. Requesting changes for two correctness regressions in the new refresh coordination, plus the lack of deterministic coverage for the synchronization contract.
Critical checkpoint conclusions:
- Goal and proof: moving RPC/deserialization out of the table monitor addresses the reported contention, but the widened sharing cohort can return a pre-commit metadata snapshot to a post-commit query. No targeted automated test proves the intended handoff behavior.
- Scope: the one-file change is focused and otherwise small.
- Concurrency and locking: task selection/replacement is protected by the table monitor; FutureTask execution is one-shot; completion safely publishes cached state; heavy work is outside the monitor; no ordinary lost wakeup, duplicate same-generation RPC, lock-order issue, or deadlock was found. The blocking issue is the semantic generation boundary, not Java publication.
- Lifecycle: the transient task starts null after deserialization, retained cycles are collectable, and catalog eviction/reset introduces no separate defect. Explicit table refresh is another trigger for the accepted stale-generation issue.
- Configuration: no configuration is added or changed.
- Compatibility: no wire schema, FE-BE variable, storage format, function symbol, EditLog format, or rolling-upgrade issue was found.
- Parallel paths: non-Arrow query binding, sink binding, remote insert, and insert-overwrite all converge on this method and inherit its task semantics; Arrow Flight is intentionally separate.
- Conditions and error handling: null/done selection, ordinary exceptional retry, and waiter interrupt restoration are mechanically sound. FutureTask nevertheless captures Error causes, and the new ExecutionException handler incorrectly converts them into ordinary Nereids AnalysisException failures.
- Tests and results: the diff contains no test; existing command tests mock getOlapTable(), and sequential Remote Doris regressions do not force the changed interleavings. The PR's cited concurrent validation ran on a downstream build and was not rerun on this head. Per the runner contract, this review was static-only and ran no local build or test.
- Observability: existing warnings are adequate for ordinary refresh failures; no independent metric issue was found. Fatal-cause misclassification is covered by the inline issue.
- Transactions, persistence, and data correctness: this class does not commit or persist data, but the returned partition visible version is installed in remote scan ranges. Reusing V after V+1 is visible violates subsequent-query visibility.
- Variable propagation: no new FE-BE or distributed variable is introduced.
- Performance and memory: removing the long monitor hold is beneficial, and per-generation task allocation/retention is negligible and no worse than the removed retained result/exception fields.
- Other risks: no additional correctness, lifecycle, compatibility, performance, observability, or coverage defect survived the final sweep.
User focus: no additional user-provided focus was supplied.
Review status: complete static review. Two full convergence rounds finished; both complete-review agents and the separate risk-focused agent returned NO_NEW_VALUABLE_FINDINGS in round 2, and every candidate was accepted, deduplicated, or dismissed with concrete evidence.
| // Remove redundant nested synchronized block | ||
| tableId = olapTable.getId(); | ||
| partitions = Lists.newArrayList(olapTable.getPartitions()); | ||
| tempPartitions = Lists.newArrayList(olapTable.getTempPartitions().getPartitions()); |
There was a problem hiding this comment.
[P1] Preserve the post-commit metadata boundary
isDone() lets a caller that starts after a remote transaction becomes visible join a refresh whose server-side snapshot predates that commit. FrontendServiceImpl.getOlapTableMeta() snapshots table/partition metadata under the remote table read lock; after that lock is released, V+1 can become visible while this task is still transporting or rebuilding a large response. A later query then selects this unfinished task and plans with version V, so committed rows can be invisible to a subsequent query. The removed monitor scope forced that later caller to wait and issue a new post-commit RPC, and the existing Remote Doris regression suite explicitly expects a select after insert to fetch the new partition version. Please preserve a generation/invalidation boundary that prevents post-snapshot callers from reusing earlier metadata.
This coordination protocol also has no deterministic upstream test: existing command tests mock getOlapTable(), and the cited manual concurrency run was not performed on this head. Please add latch/barrier-based FE coverage for one RPC/result within a shareable cohort, a fresh successor after completion or invalidation (including the V-to-V+1 schedule above), retry after exceptional completion, and interruption of one waiter without cancelling the shared task.
There was a problem hiding this comment.
Thanks for the review. I do not consider the previous locking behavior a
post-commit consistency boundary.
The old implementation explicitly intended concurrent callers to reuse an
in-flight refresh through isSyncOlapTable, remoteOlapTable, and wait/notifyAll.
The reuse failed because makeSureInitialized() and the refresh section competed
for the same monitor. Although makeSureInitialized() released the monitor before
calling getDorisOlapTable(), the same running thread could immediately reacquire
it before the other awakened threads were scheduled. It then started another
long refresh and blocked those threads again. This monitor barging caused the
repeated serial refreshes observed in production; it was not an intentional
metadata-generation mechanism.
This PR restores the original single-flight intent: the monitor only selects a
FutureTask, while RPC and metadata reconstruction run outside it. Callers that
overlap an unfinished refresh share its result. Completed results are not
cached, so the next non-overlapping request always starts a new refresh.
A normal sequential INSERT followed by SELECT remains correct: the metadata
refresh used during INSERT planning finishes before execution and commit, so the
following SELECT sees a completed task and starts a new refresh. The reported
V-to-V+1 case requires an unrelated refresh and commit to overlap. Remote Doris
Catalog does not currently guarantee linearizable metadata snapshots across
such concurrent operations or remote FE nodes. Such a guarantee would require an
explicit metadata version or invalidation protocol, not reliance on accidental
monitor scheduling.
Therefore, I do not plan to add the proposed generation boundary or encode that
new consistency guarantee in this PR.
| olapTable.setDatabase((RemoteDorisExternalDatabase) db); | ||
|
|
||
| tableId = olapTable.getId(); | ||
| partitions = Lists.newArrayList(olapTable.getPartitions()); |
There was a problem hiding this comment.
[P1] Preserve fatal Error propagation
FutureTask.run() captures every Throwable, so an OutOfMemoryError while deserializing/copying the large partition set (or another JVM Error) reaches this branch as the ExecutionException cause. Wrapping it unconditionally in Nereids AnalysisException turns a fatal VM/invariant failure into an ordinary query-analysis error; on the direct query path that materially changes control flow, and the old catch (Exception) implementation did not catch the creator's Error. Please rethrow Error causes unchanged before wrapping expected refresh exceptions, and add a focused test for this boundary.
There was a problem hiding this comment.
The previous implementation did not explicitly handle Error either, nor did it provide consistent Error propagation across the refreshing and waiting threads.
More importantly, Errors such as OutOfMemoryError or StackOverflowError indicate a JVM-level failure rather than a recoverable metadata-refresh failure. Once such an Error occurs, the health of the entire FE process is already in question; preserving a specific propagation path in this method does not provide a meaningful correctness guarantee for this API.
This PR targets lock contention during normal metadata refreshes. It does not change metadata correctness or recoverable exception handling. Therefore, JVM-fatal Error propagation is outside the scope of this fix and should not be classified as a P1 issue.
e6fc9e5 to
a33291d
Compare
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 16964 ms |
TPC-DS: Total hot run time: 83140 ms |
ClickBench: Total hot run time: 14.95 s |
|
/review |
There was a problem hiding this comment.
Exact-head static review completed for 6e8c98d4c7cf6e8ae01488fcfe388870d17ae956.
No additional non-duplicate inline findings were found. The two existing inline discussions remain the applicable concerns on this head and were not reposted: the metadata generation/invalidation boundary (discussion_r4024408493) and fatal Error propagation through FutureTask (discussion_r4024408497). This review does not supersede those discussions.
Critical checkpoint conclusions:
- Goal and proof: the patch narrows the table monitor to task selection and runs remote RPC/deserialization outside it. The added JUnit coverage proves one in-flight task/result is shared, a completed task gets a fresh successor, and an exceptional task can be retried. Per the review-runner instructions, builds and tests were not executed here, so verification is static only.
- Scope and clarity: the change is focused on one coordination method plus its unit test; no unrelated production behavior was added.
- Concurrency: FE planning threads are the concurrent callers.
currentRefreshTaskis accessed under the table monitor, callers retain a stable local task reference, andFutureTaskcompletion publishes the constructed result. Expensive RPC, reconstruction, and backend-cache inspection are outside the monitor. No new lock-order or deadlock issue was found. - Lifecycle: successful and exceptional tasks are replaced after terminal completion; an interrupted waiter restores its interrupt status without cancelling the shared task; the task field is transient. The known post-snapshot/post-invalidation reuse schedule is already covered by the existing generation-boundary thread.
- Configuration and compatibility: no configuration, FE/BE protocol, persisted format, function symbol, or rolling-upgrade contract changes are introduced.
- Parallel paths: query binding, sink binding, normal remote insert, and insert-overwrite all resolve through the same refresh protocol; no missing parallel implementation was found.
- Conditions and errors: runtime refresh failures are logged once, shared through the task, wrapped with their cause, and retried by a successor task. The distinct fatal-
Errorbehavior is already covered by the existing thread. - Tests and results: the latch-based test is deterministic for the covered cohort and the retry assertion is sound. The existing discussions already fence the missing generation/invalidation, interruption, and fatal-
Errorboundary cases. No generated result files are involved. - Observability: the warning includes database and table identifiers; no new metric or additional INFO logging is warranted for this local coordination change.
- Transactions, persistence, and data writes: the patch changes metadata-read coordination only; it adds no EditLog state, transaction transition, data-write operation, or new FE-to-BE variable.
- Performance: the long metadata work is removed from the table monitor without adding an executor, background task, redundant scan, or new steady-state cache. No additional performance defect was found.
- User focus: no additional focus points were supplied; the complete two-file change was reviewed.
Review convergence: two normal full-review passes and one risk-focused pass all returned NO_NEW_VALUABLE_FINDINGS; the main-agent final changed-file and unresolved-candidate sweep found no new comment to submit.
3 participants
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
Concurrent queries against the same Remote Doris Catalog table with
use_arrow_flight=falsecan stall during FE planning.Two factors compound the problem:
makeSureInitialized()and metadata refresh share the same table monitor, which the refresh holds throughout RPC, deserialization, partition reconstruction, and cache updates. Arriving requests block at initialization before reaching the reuse branch. After a refresh completes, a thread leaving initialization may immediately reacquire the monitor for another refresh while others remain blocked, limiting result reuse.This PR uses a
FutureTask<RemoteOlapTable>to represent one metadata refresh:AnalysisExceptionwith the original cause. An interrupted waiter restores its interrupt status without cancelling the shared task.The scope is one
RemoteDorisExternalTableinstance within one FE process. The metadata RPC parameters, partition synchronization, and cache update logic remain unchanged.Release note
None
Check List (For Author)
Manual test:
Concurrent-query validation was performed on a downstream build with the same refresh-coordination change, using temporary diagnostic logs excluded from this PR. It has not been rerun on this upstream branch.
Steps to reproduce the validation:
Create a Remote Doris Catalog with
use_arrow_flight=false.Disable SQL Cache in every query connection:
Run concurrent queries against the same remote table.
With temporary instrumentation around task selection and execution, group logs by FE and table instance. Verify that requests selecting an unfinished task share its task identifier and only its creator runs the refresh.
Behavior changed:
No.
Yes.
Requests selecting an in-progress refresh now share that task. After completion, the next task selection starts a new refresh. Refresh failures are consistently wrapped in
AnalysisException, and interrupted waiters restore their interrupt status.Does this need documentation?
Check List (For Reviewer who merge this PR)