github-actions[bot] commented on code in PR #68196:
URL: https://github.com/apache/doris/pull/68196#discussion_r4059220221
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalCatalog.java:
##########
@@ -430,7 +434,8 @@ private void buildMetaCache() {
localDbName -> Optional.ofNullable(
buildDbForInit(null, localDbName,
Util.genIdByName(name, localDbName), logType,
true)),
- (key, value, cause) -> value.ifPresent(v ->
v.resetMetaToUninitialized()),
+ (key, value, cause) -> value.ifPresent(
+ v ->
v.resetMetaToUninitialized(shouldInvalidateRowCountOnDatabaseRemoval())),
Review Comment:
[P1] Preserve routed DB invalidation on the `invalid_cache=false` path.
During `onRefreshCache(false)`, this listener receives `false`, but that flag
makes `resetMetaToUninitialized` skip the entire
`ExternalMetaCacheMgr.invalidateDb` call—not only its row-count scan. The false
branch then performs only a direct catalog row-count fence, so
schema/partition/file engine entries survive even though the DB objects were
rebuilt; before this change every removal callback routed `invalidateDb`.
Please separate row-count suppression from routed invalidation (or perform one
equivalent routed bulk invalidation), and test a real removal
callback/recording engine rather than only a mocked `MetaCache.invalidateAll()`.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/ExternalMetaCacheMgr.java:
##########
@@ -669,15 +739,32 @@ public ExternalRowCountCache getRowCountCache() {
}
public void invalidateTableCache(ExternalTable dorisTable) {
- invalidateTable(dorisTable.getCatalog().getId(),
- dorisTable.getDbName(),
- dorisTable.getName());
+ long catalogId = dorisTable.getCatalog().getId();
+ try {
+ routeCatalogEngines(catalogId, cache -> safeInvalidate(
+ cache, catalogId, "invalidateTableCache",
+ () -> cache.invalidateTable(catalogId,
dorisTable.getDbName(), dorisTable.getName())));
+ } finally {
+ invalidateRowCountCache(dorisTable);
Review Comment:
[P1] Fence the held table before the default post-commit refresh re-resolves
it. Iceberg and Paimon reach this new row-count fence only through
`handleRefreshTable(..., true)`; after the transaction is already `COMMITTED`,
an evicted table object must be rebuilt through fallible remote name/existence
checks, and a null result makes that method return without this fence or the
follower refresh log. Table eviction has no row-count removal listener, while
the executor still owns the exact committed `ExternalTable`. Invalidate that
identity in a commit-safe path before re-resolution, then handle
refresh/logging failures separately and add an evicted-table/reload-failure
regression.
##########
fe/fe-core/src/test/java/org/apache/doris/catalog/RefreshManagerTest.java:
##########
@@ -0,0 +1,121 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.catalog;
+
+import org.apache.doris.datasource.CatalogMgr;
+import org.apache.doris.datasource.ExternalCatalog;
+import org.apache.doris.datasource.ExternalDatabase;
+import org.apache.doris.datasource.ExternalMetaCacheMgr;
+import org.apache.doris.datasource.ExternalObjectLog;
+import org.apache.doris.datasource.hive.HMSExternalCatalog;
+import org.apache.doris.datasource.hive.HMSExternalTable;
+import org.apache.doris.datasource.hive.HiveExternalMetaCache;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+import java.util.Optional;
+
+public class RefreshManagerTest {
+
+ @Test
+ void testColdDatabaseReplayInvalidatesCatalogRowCount() {
+ long catalogId = 51L;
+ ExternalCatalog catalog = Mockito.mock(ExternalCatalog.class);
+ Mockito.when(catalog.getId()).thenReturn(catalogId);
+
Mockito.when(catalog.getDbForReplay("db1")).thenReturn(Optional.empty());
+ CatalogMgr catalogMgr = Mockito.mock(CatalogMgr.class);
+ Mockito.doReturn(catalog).when(catalogMgr).getCatalog(catalogId);
+ ExternalMetaCacheMgr cacheMgr =
Mockito.mock(ExternalMetaCacheMgr.class);
+ Env env = Mockito.mock(Env.class);
+ Mockito.when(env.getCatalogMgr()).thenReturn(catalogMgr);
+ Mockito.when(env.getExtMetaCacheMgr()).thenReturn(cacheMgr);
+
+ try (MockedStatic<Env> mockedEnv = Mockito.mockStatic(Env.class)) {
+ mockedEnv.when(Env::getCurrentEnv).thenReturn(env);
+ new
RefreshManager().replayRefreshDb(ExternalObjectLog.createForRefreshDb(catalogId,
"db1"));
+ }
+
+ Mockito.verify(cacheMgr).invalidateRowCountCache(catalogId);
+ }
+
+ @Test
+ void testPartitionReplayInvalidatesRowCountBeforeCacheFailure() {
+ long catalogId = 52L;
+ HMSExternalCatalog catalog = Mockito.mock(HMSExternalCatalog.class);
+ ExternalDatabase<?> db = Mockito.mock(ExternalDatabase.class);
+ HMSExternalTable table = Mockito.mock(HMSExternalTable.class);
+ Mockito.when(catalog.getId()).thenReturn(catalogId);
+
Mockito.when(catalog.getDbForReplay("db1")).thenReturn(Optional.of(db));
+
Mockito.doReturn(Optional.of(table)).when(db).getTableForReplay("tbl1");
+
+ CatalogMgr catalogMgr = Mockito.mock(CatalogMgr.class);
+ Mockito.doReturn(catalog).when(catalogMgr).getCatalog(catalogId);
+ ExternalMetaCacheMgr cacheMgr =
Mockito.mock(ExternalMetaCacheMgr.class);
+ HiveExternalMetaCache hiveCache =
Mockito.mock(HiveExternalMetaCache.class);
+ Mockito.when(cacheMgr.hive(catalogId)).thenReturn(hiveCache);
+ Mockito.doThrow(new IllegalStateException("partition cache failure"))
+ .when(hiveCache).refreshAffectedPartitionsCache(
+ Mockito.eq(table), Mockito.anyList(),
Mockito.anyList());
+ Env env = Mockito.mock(Env.class);
+ Mockito.when(env.getCatalogMgr()).thenReturn(catalogMgr);
+ Mockito.when(env.getExtMetaCacheMgr()).thenReturn(cacheMgr);
+
+ ExternalObjectLog log = ExternalObjectLog.createForRefreshPartitions(
+ catalogId, "db1", "tbl1",
+ java.util.Collections.singletonList("p=1"),
java.util.Collections.emptyList(), 1L);
+ try (MockedStatic<Env> mockedEnv = Mockito.mockStatic(Env.class)) {
+ mockedEnv.when(Env::getCurrentEnv).thenReturn(env);
+ Assertions.assertThrows(IllegalStateException.class,
+ () -> new RefreshManager().replayRefreshTable(log));
+ }
+
+ Mockito.verify(cacheMgr).invalidateRowCountCache(table);
+ }
+
+ @Test
+ void testAlterPartitionInvalidatesRowCountBeforeCacheFailure() throws
Exception {
+ long catalogId = 53L;
+ HMSExternalCatalog catalog = Mockito.mock(HMSExternalCatalog.class);
+ ExternalDatabase<?> db = Mockito.mock(ExternalDatabase.class);
+ HMSExternalTable table = Mockito.mock(HMSExternalTable.class);
+ Mockito.when(catalog.getId()).thenReturn(catalogId);
+ Mockito.doReturn(db).when(catalog).getDbNullable("db1");
+ Mockito.when(db.getTableNullable("tbl1")).thenReturn(table);
Review Comment:
[P1] Use a wildcard-safe Mockito stub here. `db` is declared as
`ExternalDatabase<?>`, so `getTableNullable` returns a captured wildcard and
`when(...).thenReturn(table)` cannot accept an `HMSExternalTable`; the test
source will not compile. The same pattern is added in `CatalogMgrTest` at lines
223 and 251. Please use
`Mockito.doReturn(table).when(db).getTableNullable("tbl1")` (as the nearby
wildcard-returning stubs do), or give the mock a concrete compatible type
parameter.
--
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]
