github-actions[bot] commented on code in PR #67813:
URL: https://github.com/apache/doris/pull/67813#discussion_r4022540910
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudPartition.java:
##########
@@ -127,9 +132,13 @@ public long getCachedVisibleVersion() {
return super.getVisibleVersion();
}
+ public void invalidateCachedVisibleVersion() {
+ versionCacheEpoch.incrementAndGet();
+ }
+
@VisibleForTesting
protected boolean isCachedVersionExpired() {
Review Comment:
[P1] Read the cached value after validating its epoch
The batch readers currently load the numeric cache before calling this epoch
check. After invalidation leaves epoch 1/refreshed 0 and value v2, a concurrent
single-item `getVisibleVersion()` can fetch v3, write it, and publish refreshed
epoch 1 between those two operations. The batch then sees this check pass but
returns its earlier local v2; `ScanNode.setVisibleVersionForOlapScanNodes` uses
that stale value directly even for a one-partition query. This is distinct from
the partial-subset race: the epoch never changes, so a later epoch recheck
would still pass. In both partition and table batch readers, establish that the
epoch is valid first and only then load the cached value (or make every
epoch-acknowledging publisher take the matching version write lock), and add a
deterministic interleaving test.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudPartition.java:
##########
@@ -266,15 +280,27 @@ public static List<Long> getSnapshotVisibleVersionFromMs(
// Cache visible version, see hasData() for details.
int size = versions.size();
boolean hasCommitTsos = commitTsos.size() == size;
- for (int i = 0; i < size; ++i) {
- Long version = versions.get(i);
- if (version > Partition.PARTITION_INIT_VERSION) {
- // For compatibility, the existing partitions may not have
mtime
- long mTime = versions.size() == versionUpdateTimesMs.size() ?
versionUpdateTimesMs.get(i) : 0;
Review Comment:
[P1] Recheck cached siblings after a partial refresh
This write lock protects only the partitions passed in the expired subset,
but the caller may already have copied other siblings from cache and released
its read lock. For example, P1 is expired and P2 is cached at v2; after that
snapshot, a VISIBLE retry invalidates both epochs, then this one-partition RPC
returns P1=v3. The caller splices `[v3, v2]`, and
`ScanNode.setVisibleVersionForOlapScanNodes` sends that impossible mixed
snapshot to the BEs even though the transaction made both partitions visible
atomically. This is distinct from the prior unlocked-writer thread because the
omitted sibling is outside this new lock. Fetch the full requested batch when
any entry expires, or recheck every copied epoch and retry after intervening
invalidation; apply the same rule to the partial table-version batch.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -3605,13 +3609,18 @@ protected boolean isCachedTableVersionExpired() {
public boolean isCachedTableVersionExpired(long expirationMs) {
// -1 means no cache yet, need to fetch from MS
Review Comment:
[P2] Let the daemon complete the invalidation epoch
Once this predicate sees an epoch mismatch, `CloudSyncVersionDaemon` cannot
make it false: the daemon reads authoritative table versions through raw
`getVisibleVersionFromMeta`, then calls only `setCachedTableVersion`, which
never advances `refreshedTableVersionCacheEpoch`. The table is therefore
selected for an MS RPC on every later daemon pass (and by every cache-aware
table-version consumer until a foreground wrapper repairs it). If a delayed
normal notification already supplied the same numeric table version, the daemon
also skips partition refresh, leaving all partition epochs mismatched. Capture
each epoch before the daemon RPC, refresh invalidated partitions even when the
numeric table version is unchanged, and acknowledge only that captured epoch
after the paired refresh succeeds so a concurrent invalidation remains pending.
--
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]
