[enhance](cloud) Increase get version timeout to 30 seconds#65625
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found two blocking issues with raising the default cloud get-version timeout to 30 seconds. Both are availability/lifecycle problems where this retrying MetaService call is used inside existing critical sections.
Critical checkpoints: the stated goal is clear, but the implementation is not safe in all affected call paths; the change is small but broad in effect; config compatibility is restart/default-only and not the primary blocker; no storage/wire compatibility issue was found; tests were not run in this review environment per instructions, and I did not find coverage for these timeout/critical-section interactions. No additional user focus was provided.
bd1e485 to
bf5f448
Compare
|
run buildall |
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
TPC-H: Total hot run time: 29364 ms |
TPC-DS: Total hot run time: 177353 ms |
ClickBench: Total hot run time: 25.03 s |
|
/review |
|
PR approved by anyone and no changes requested. |
There was a problem hiding this comment.
Request changes.
Critical-checkpoint conclusions:
- Goal, scope, and correctness: the one-line change lets one default MetaService get-version RPC wait up to 30 seconds, but
VersionHelperapplies it per attempt across up to 200 retries and every caller shares it. The PR therefore does not safely achieve its stated slow-response goal. Version decoding, cloud visible-version normalization, and data-visibility semantics themselves are unchanged; I found no separate committed/uncommitted-data or version-consistency corruption. - Concurrency and lifecycle: no new data race, deadlock cycle, static-initialization dependency, or ownership leak was introduced. The material regression is lock/request occupancy: in addition to the already-reported delete-bitmap lease and SHOW metadata read lock, foreground cancellation is ineffective, cloud export serializes RPCs under a read lock, and cloud DDL/restore paths can RPC under exclusive table locks. Background sync uses dedicated finite workers and gates table-cache advancement on successful partition batches, so no separate partial-state bug was substantiated there.
- Configuration, compatibility, persistence, and parallel paths: no new config key, FE-BE field, wire/storage-format change, or rolling-upgrade incompatibility is introduced. This restart-only global default affects single and batch version fetches across query, export, DDL, restore, proc, transaction, and daemon paths rather than a caller-specific budget. Any fix must preserve DROP PARTITION
versionTimelogging and restore/alter state revalidation. - Performance and observability: the default worst-case wait becomes about 6,000 seconds plus retry sleeps, and serial partition/tablet callers amplify successful slow waits. Existing logs, RPC metrics, and query summary timing observe the latency but do not bound it or make cancellation effective.
- Tests: the PR reports compilation/checkstyle only and adds no functional test for a slow/never-completing MetaService Future, caller deadline/cancellation, cold-cache table-lock release, export fan-out, or restore-after-failover. No result files changed. Per the review-runner instruction, this was a static review; no build or test was run here.
- User focus: no additional focus was provided; the complete one-line change and all discovered caller families were reviewed.
Three new inline findings are attached. The two existing Config.java threads (SHOW metadata lock and cloud MoW delete-bitmap lock) were treated as known context and not duplicated. The PR is not ready until the get-version wait is bounded per operation/caller and MetaService RPCs are removed from the identified lock-held regions.
|
|
||
| @ConfField | ||
| public static int default_get_version_from_ms_timeout_second = 3; | ||
| public static int default_get_version_from_ms_timeout_second = 30; |
There was a problem hiding this comment.
This is a per-attempt timeout, so foreground version fetches can now escape their caller deadlines. VersionHelper.getVisibleVersion() applies it up to the default 200 times (about 100 minutes of waits). On a cold/expired cache, cloud Nereids planning synchronously reaches ScanNode.setVisibleVersionForOlapScanNodes() before a coordinator exists; after query_timeout, ConnectContext.checkTimeout() calls StmtExecutor.cancel(), but coord is null and the Future/retry loop keeps running. With cloud_partition_version_cache_ttl_ms <= 0 (or an expired cache), the short-circuit path can also call updateCloudPartitionVersions() before PointQueryExecutor creates its 10-second timeoutTs, and cancel(Status) is a no-op. Thus a 5-second query timeout can spend 30 seconds on one attempt and continue retrying. This is distinct from the existing lock-lifetime threads. Please pass one absolute caller deadline/cancellation signal into the version retry loop (and stop swallowing interruption), and add a never-completing-Future test proving foreground requests terminate within that deadline.
|
|
||
| @ConfField | ||
| public static int default_get_version_from_ms_timeout_second = 3; | ||
| public static int default_get_version_from_ms_timeout_second = 30; |
There was a problem hiding this comment.
Cloud EXPORT can serially multiply this enlarged wait while holding a table read lock, which is a distinct path from the existing batched SHOW PARTITIONS thread. ExportJob.generateOutfileLogicalPlans() keeps exportTable.readLock() across splitTablets(): for each of up to 2,000 partitions whose local version does not already prove it nonempty, CloudPartition.hasData() can issue a single-partition get-version RPC. With data_consistency=none, the initial consistency snapshot also calls getVisibleVersion() for every selected partition in that lock scope. ExportTaskExecutor later checks versions per tablet under the read lock as well (a nonpositive TTL can make those calls RPC too). One failed lookup alone can spend 30 seconds on each of 200 attempts, while slow successes repeat serially and exclude DDL writers. Please snapshot partition/tablet identities under the lock, batch the unique version fetches outside it under an export-level deadline, then reacquire and revalidate the identities/versions needed for consistency. Add a cold-cache/slow-MetaService cloud export test proving the write lock remains obtainable during the RPC.
|
|
||
| @ConfField | ||
| public static int default_get_version_from_ms_timeout_second = 3; | ||
| public static int default_get_version_from_ms_timeout_second = 30; |
There was a problem hiding this comment.
Exclusive cloud DDL/restore paths also inherit this 30-second-per-attempt policy. DROP PARTITION holds olapTable.writeLock() through InternalCatalog.dropPartitionWithoutCheck(), where partition.hasData() can fetch a cold cloud version before the drop log is written. CloudSchemaChangeJobV2 and CloudRollupJobV2 inherit running phases that take the table write lock and then call partition.getVisibleVersion() per partition before committing. When cloud restore is enabled, a resumed/replayed CloudRestoreJob inherits RestoreJob.allTabletCommitted(), which holds tbl.writeLockIfExist() while a no-op cloud updateVersionForRestore() is followed by getVisibleVersion() and replica updates; after restart/failover its nonserialized cache timestamp is cold. These paths can enter 30 seconds x 200 retries while all readers/writers are excluded. Please fetch/batch the required cloud version/has-data state outside the write lock under the operation deadline, then reacquire and revalidate table/partition/job state before mutating or logging (preserving drop versionTime semantics).
### What problem does this PR solve? The default timeout for FE get_version requests to MetaService is 3 seconds, which is too short when MetaService responds slowly. Increase the default timeout to 30 seconds to reduce premature request failures.
### What problem does this PR solve? The default timeout for FE get_version requests to MetaService is 3 seconds, which is too short when MetaService responds slowly. Increase the default timeout to 30 seconds to reduce premature request failures.
4 participants
What problem does this PR solve?
The default timeout for FE get_version requests to MetaService is 3 seconds, which is too short when MetaService responds slowly. Increase the default timeout to 30 seconds to reduce premature request failures.
Release note
Increase the default FE get_version timeout from 3 seconds to 30 seconds.
Check List (For Author)
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)