Copilot commented on code in PR #60064:
URL: https://github.com/apache/doris/pull/60064#discussion_r2707586855
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -3301,8 +3302,9 @@ public boolean isCachedTableVersionExpired() {
return System.currentTimeMillis() - lastTableVersionCachedTimeMs >
cacheExpirationMs;
}
- public void setCachedTableVersion(long version) {
- if (version > cachedTableVersion) {
+ @VisibleForTesting
+ protected void setCachedTableVersion(long version) {
+ if (version >= cachedTableVersion) {
Review Comment:
Changing the condition from `version > cachedTableVersion` to `version >=
cachedTableVersion` breaks the existing test at line 380-386 of
OlapTableTest.java. The test verifies that calling setCachedTableVersion with
an equal or smaller version should NOT update the timestamp, preventing
incorrect cache TTL resets. With this change, setting the same version will
refresh the cache timestamp, which is not the intended behavior and will cause
the test to fail.
```suggestion
if (version > cachedTableVersion) {
```
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudPartition.java:
##########
@@ -251,16 +261,18 @@ public static List<Long> getSnapshotVisibleVersionFromMs(
// Return the visible version in order of the specified partition ids, -1
means version NOT FOUND.
public static List<Long> getSnapshotVisibleVersion(List<CloudPartition>
partitions) throws RpcException {
if (partitions.isEmpty()) {
- return new ArrayList<>();
+ return Collections.emptyList();
}
- if (SessionVariable.cloudPartitionVersionCacheTtlMs <= 0) { // No
cached versions will be used
+ long cloudPartitionVersionCacheTtlMs = ConnectContext.get() == null ? 0
+ :
ConnectContext.get().getSessionVariable().cloudPartitionVersionCacheTtlMs;
+ if (cloudPartitionVersionCacheTtlMs <= 0) { // No cached versions will
be used
return getSnapshotVisibleVersionFromMs(partitions, false);
}
Review Comment:
Potential NullPointerException: `ConnectContext.get()` is called twice on
lines 267-268. If the context becomes null between the first check and the
second call, this will throw a NullPointerException. Store the result in a
variable to avoid calling it twice.
```suggestion
ConnectContext ctx = ConnectContext.get();
long cloudPartitionVersionCacheTtlMs = ctx == null ? 0
: ctx.getSessionVariable().cloudPartitionVersionCacheTtlMs;
if (cloudPartitionVersionCacheTtlMs <= 0) { // No cached versions
will be used
return getSnapshotVisibleVersionFromMs(partitions, false);
```
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -3367,14 +3369,21 @@ public static List<Long>
getVisibleVersionInBatch(Collection<OlapTable> tables)
.collect(Collectors.toList());
}
- List<Long> dbIds = new ArrayList<>();
- List<Long> tableIds = new ArrayList<>();
+ List<Long> dbIds = new ArrayList<>(tables.size());
+ List<Long> tableIds = new ArrayList<>(tables.size());
for (OlapTable table : tables) {
dbIds.add(table.getDatabase().getId());
tableIds.add(table.getId());
}
- return getVisibleVersionFromMeta(dbIds, tableIds);
+ List<Long> versions = getVisibleVersionFromMeta(dbIds, tableIds);
+
+ // update cache
+ Preconditions.checkState(tables.size() == versions.size());
+ for (int i = 0; i < tables.size(); i++) {
+ tables.get(i).setCachedTableVersion(versions.get(i));
+ }
Review Comment:
The new cache update logic in lines 3381-3385 lacks test coverage. Since
OlapTableTest.java already has comprehensive tests for cache behavior
(testTableVersionCacheWithRpc), consider adding a test for
getVisibleVersionInBatch to verify that the cache is correctly updated when
fetching versions in batch mode.
--
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]
