Copilot commented on code in PR #68104:
URL: https://github.com/apache/doris/pull/68104#discussion_r4032818011
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -3594,39 +3596,61 @@ public long getNextVersion() {
@VisibleForTesting
protected boolean isCachedTableVersionExpired() {
- // -1 means no cache yet, need to fetch from MS
- if (cachedTableVersion == -1 || tableVersionCacheEpoch.get() !=
refreshedTableVersionCacheEpoch.get()) {
- return true;
- }
ConnectContext ctx = ConnectContext.get();
long cacheExpirationMs = ctx == null ?
VariableMgr.getDefaultSessionVariable().cloudTableVersionCacheTtlMs
: ctx.getSessionVariable().cloudTableVersionCacheTtlMs;
- if (cacheExpirationMs <= 0) { // always expired
- return true;
- }
- return System.currentTimeMillis() - lastTableVersionCachedTimeMs >
cacheExpirationMs;
+ return isCachedTableVersionExpired(cacheExpirationMs);
}
public boolean isCachedTableVersionExpired(long expirationMs) {
// -1 means no cache yet, need to fetch from MS
- if (cachedTableVersion == -1 || expirationMs <= 0
+ if (!Config.cloud_enable_version_syncer
+ || cachedTableVersion == -1 || expirationMs <= 0
|| tableVersionCacheEpoch.get() !=
refreshedTableVersionCacheEpoch.get()) {
Review Comment:
When a table is marked `partitionVersionSyncNeeded`, a finite TTL can still
make `getVisibleVersion()` fetch and cache the newer MS table version before
the daemon publishes its staged partitions. `SqlCacheContext.addUsedTable()`
records that table version, while the query can still read the old partition
snapshot, and the SQL cache validates only this table token; this can retain a
stale result under the newer version. Coordinate table-version reads with the
pending publication (or validate/retry the complete SQL snapshot) instead of
treating only TTL/epoch as cache expiry.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudPartition.java:
##########
@@ -257,6 +265,29 @@ public static List<Long> getSnapshotVisibleVersionFromMs(
public static List<Long> getSnapshotVisibleVersionFromMs(
List<CloudPartition> partitions, boolean waitForPendingTxns, int
maxAttempts) throws RpcException {
+ List<PartitionVersion> snapshots =
getSnapshotVisibleVersionFromMsWithoutCache(
+ partitions, waitForPendingTxns, maxAttempts);
+ List<Long> versions = new ArrayList<>(snapshots.size());
+ List<OlapTable> tables = getTables(partitions);
+ for (OlapTable table : tables) {
+ table.versionWriteLock();
+ }
+ try {
Review Comment:
The new write-lock publication does not make a foreground partition snapshot
atomic: `getSnapshotVisibleVersion()` releases its table read lock after
recording cached versus expired partitions, then performs the MS RPC and
reacquires the write lock only to update the expired subset. A daemon refresh
can publish between those phases, so one query can combine an old cached
partition with a newer fetched partition. Revalidate the table/cache epoch and
retry the full snapshot (or otherwise keep the snapshot transactionally
consistent) before returning it.
##########
fe/fe-core/src/main/java/org/apache/doris/common/proc/PartitionsProcDir.java:
##########
@@ -422,7 +422,9 @@ private List<Long> getPartitionVersions(OlapTable
olapTable, List<Long> partitio
for (int start = 0; start < partitions.size(); start +=
batchSize) {
int end = Math.min(start + batchSize, partitions.size());
List<CloudPartition> batch = partitions.subList(start,
end);
-
partitionVersions.addAll(CloudPartition.getSnapshotVisibleVersionFromMs(batch,
false));
+ // Lazy commit may advance the table version before its
partition versions become visible.
+
partitionVersions.addAll(CloudPartition.getSnapshotVisibleVersionFromMs(
+ batch, true,
Config.cloud_version_syncer_get_version_retry_times));
Review Comment:
The table version is fetched before this partition RPC, but this call now
waits for pending transactions and can therefore return a snapshot from a later
commit. There is no second table-version read/validation before
`pushVersionAsync` below, so a commit between the table fetch and this call can
push `table=101` together with `partition=14` from table version 102.
Revalidate the table version and publish the table/partition snapshot
atomically before broadcasting it.
##########
fe/fe-core/src/main/java/org/apache/doris/common/proc/PartitionsProcDir.java:
##########
@@ -422,7 +422,9 @@ private List<Long> getPartitionVersions(OlapTable
olapTable, List<Long> partitio
for (int start = 0; start < partitions.size(); start +=
batchSize) {
int end = Math.min(start + batchSize, partitions.size());
List<CloudPartition> batch = partitions.subList(start,
end);
-
partitionVersions.addAll(CloudPartition.getSnapshotVisibleVersionFromMs(batch,
false));
+ // Lazy commit may advance the table version before its
partition versions become visible.
+
partitionVersions.addAll(CloudPartition.getSnapshotVisibleVersionFromMs(
+ batch, true,
Config.cloud_version_syncer_get_version_retry_times));
Review Comment:
Each loop iteration calls the caching overload, so a successful earlier
batch publishes its partition versions immediately. If a later batch fails,
this method throws without updating the table cache or invalidating the
already-updated partitions; subsequent reads can therefore combine a partially
refreshed partition set. Stage all batches without cache writes and publish
only after every batch succeeds, or invalidate the table and all affected
partition caches in the failure path.
--
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]
