github-actions[bot] commented on code in PR #68104:
URL: https://github.com/apache/doris/pull/68104#discussion_r4045487571
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudPartition.java:
##########
@@ -203,14 +208,37 @@ private long getVisibleVersionFromMs(boolean
waitForPendingTxns) {
if (LOG.isDebugEnabled()) {
LOG.debug("get version from meta service, version: {},
partition: {}", version, super.getId());
}
- setCachedVisibleVersion(version, mTime);
- refreshedVersionCacheEpoch.accumulateAndGet(cacheEpoch, Math::max);
+ cacheVisibleVersion(version, mTime, -1, cacheEpoch,
isVersionCacheable(resp, 0, 1));
return version;
} catch (RpcException e) {
throw new RuntimeException("get version from meta service failed");
}
}
+ private void cacheVisibleVersion(long version, long updateTimeMs, long
tso, long cacheEpoch, boolean cacheable) {
+ if (!cacheable) {
+ // Invalidate before touching the timestamp, even when the cache
was valid before this read.
+ invalidateCachedVisibleVersion();
+ }
+ setCachedVisibleVersion(version, updateTimeMs, tso);
Review Comment:
[P1] Do not publish metadata from a pending response. The MS returns the old
visible version together with the pending transaction's update time and commit
TSO. Although this branch invalidates the epoch, the unconditional setter still
installs that mismatched tuple on a lagging FE, and direct accessors such as
getVisibleVersionTime()/getTso() do not check the epoch; an equal-version push
cannot repair it because the setter only accepts a strictly newer version.
Ordinary SHOW PARTITIONS and MTMV grace checks can therefore observe metadata
for a transaction that is not visible. Return the version to this caller
without mutating shared partition state until hasPendingTxns is false, or make
the tuple validity-checked atomically.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -3190,11 +3191,15 @@ public void setDetailShapePlanNodes(String
detailShapePlanNodes) {
@VarAttrDef.VarAttr(name = CLOUD_FORCE_SYNC_TABLET_STATS, needForward =
true)
public boolean cloudForceSyncTabletStats = false;
@VarAttrDef.VarAttr(name = CLOUD_PARTITION_VERSION_CACHE_TTL_MS)
- public long cloudPartitionVersionCacheTtlMs = Long.MAX_VALUE;
+ public long cloudPartitionVersionCacheTtlMs = 10 * 60 * 1000L;
Review Comment:
[P2] Bound the foreground refresh before enabling this finite default. Once
600000 ms elapses, a query selecting P expired partitions sends all P ids in
one get-version request and allocates parallel O(P) response lists; unlike the
daemon/proc paths, this path ignores cloud_get_version_task_batch_size. Because
partitions cached together also expire together, large scans now periodically
put an unbounded metadata RPC on the SELECT path by default. Chunk this refresh
while preserving a consistent snapshot, or otherwise stagger/renew it before
changing the default.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudPartition.java:
##########
@@ -372,8 +406,7 @@ public static List<Long>
getSnapshotVisibleVersion(List<CloudPartition> partitio
List<Long> versions = null;
if (!expiredPartitions.isEmpty()) { // Not all partition versions are
from cache
- versions = getSnapshotVisibleVersionFromMs(
- expiredPartitions, /*waitForPendingTxns=*/false); // Get
the rest versions from meta-service
+ versions = getSnapshotVisibleVersionFromMs(expiredPartitions); //
Get the rest versions from meta-service
Review Comment:
[P2] Do not perform the session-enabled wait while Nereids holds the table
metadata lock. Ordinary planning retains the table read lock through
PruneEmptyPartition, which reaches this RPC on a cache miss; with
cloud_get_version_wait_for_pending_txn enabled, the call can wait/retry for
pending lazy commits for the full VersionHelper retry envelope. ADD/DROP/ALTER
PARTITION then blocks behind a SELECT planner. Snapshot ids under the lock,
wait outside it, and reacquire/revalidate, or move this wait to a post-lock
snapshot phase.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudSyncVersionDaemon.java:
##########
@@ -154,6 +148,24 @@ private Future<Void>
submitGetTableVersionTask(Map<OlapTable, Long> tableVersion
});
}
+ private void prepareTableForVersionSync(Map<OlapTable, Long>
tableVersionMap, OlapTable table, long version) {
+ // Stop serving potentially stale partition caches while refresh is
queued or fails.
+ table.readLock();
+ try {
+ table.versionWriteLock();
+ try {
+ for (Partition partition : table.getAllPartitions()) {
Review Comment:
[P2] Avoid the unbounded partition sweep under the table metadata lock. Each
table-version advance now holds table.readLock (and the version write lock)
while invalidating every partition, so ADD/DROP/ALTER PARTITION is blocked for
O(partition count) work; if the later refresh fails, the cached table token is
not advanced and the same locked sweep repeats next cycle. Snapshot the
partition objects under the metadata lock and invalidate outside it, or use a
table-level generation/bounded publication scheme.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -3190,11 +3191,15 @@ public void setDetailShapePlanNodes(String
detailShapePlanNodes) {
@VarAttrDef.VarAttr(name = CLOUD_FORCE_SYNC_TABLET_STATS, needForward =
true)
public boolean cloudForceSyncTabletStats = false;
@VarAttrDef.VarAttr(name = CLOUD_PARTITION_VERSION_CACHE_TTL_MS)
Review Comment:
[P1] Migrate the persisted old default on upgrade. The FE image serializes
this annotated long as part of defaultSessionVariable, and image load assigns
the saved Long.MAX_VALUE over the initializer; forceUpdateVariables has no
migration for this key. Existing clusters that never changed the setting
therefore keep an infinite TTL, so the missed-update fallback enabled by this
PR only applies to fresh clusters. Add a versioned migration with an explicit
policy for real operator overrides and an old-image replay test, or document
and scope the change as new-cluster-only.
--
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]
