github-actions[bot] commented on code in PR #65625:
URL: https://github.com/apache/doris/pull/65625#discussion_r3600255175
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -3071,7 +3071,7 @@ public static int metaServiceRpcRetryTimes() {
public static int drop_rpc_retry_num = 200;
@ConfField
- public static int default_get_version_from_ms_timeout_second = 3;
+ public static int default_get_version_from_ms_timeout_second = 30;
Review 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 in
terruption), and add a never-completing-Future test proving foreground
requests terminate within that deadline.
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -3071,7 +3071,7 @@ public static int metaServiceRpcRetryTimes() {
public static int drop_rpc_retry_num = 200;
@ConfField
- public static int default_get_version_from_ms_timeout_second = 3;
+ public static int default_get_version_from_ms_timeout_second = 30;
Review 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 rev
alidate the identities/versions needed for consistency. Add a
cold-cache/slow-MetaService cloud export test proving the write lock remains
obtainable during the RPC.
##########
fe/fe-common/src/main/java/org/apache/doris/common/Config.java:
##########
@@ -3071,7 +3071,7 @@ public static int metaServiceRpcRetryTimes() {
public static int drop_rpc_retry_num = 200;
@ConfField
- public static int default_get_version_from_ms_timeout_second = 3;
+ public static int default_get_version_from_ms_timeout_second = 30;
Review 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 an
d revalidate table/partition/job state before mutating or logging (preserving
drop `versionTime` semantics).
--
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]
