github-actions[bot] commented on code in PR #66913:
URL: https://github.com/apache/doris/pull/66913#discussion_r3877826601
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -2143,8 +2196,8 @@ public static List<Column>
getIcebergPartitionColumns(Optional<MvccSnapshot> sna
if (snapshotTable.isPresent()) {
// Schema ID alone cannot identify the partition spec;
metadata-only evolution may keep
// the same schema and snapshot IDs while changing spec(), so
derive both from T0.
- return buildTableSchemaCacheValue(dorisTable,
snapshotValue.getSnapshot().getSchemaId(),
- snapshotTable.get()).getPartitionColumns();
+ return getSchemaCacheValue(dorisTable,
snapshotValue.getSnapshot().getSchemaId(),
Review Comment:
[P2] Match historical partition columns by field ID
This newly caches the historical schema projection, but its loader resolves
each partition source in the current `icebergTable.schema()` and matches that
current name to the historical Doris schema by name. After renaming a partition
source column, the old schema keeps the same field ID under its old name, so a
cold lookup returns no partition column; the new cache can also retain the
opposite result across rename/rename-back. Native historical scans then lose
pruning, while partition-aware MTMV consumers can fail or misclassify the
table. Match `Column.getUniqueId()` to `PartitionField.sourceId()` and cover a
pre-rename snapshot with cold and warm caches.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalCatalog.java:
##########
@@ -173,17 +221,108 @@ protected List<String>
listTableNamesFromRemote(SessionContext ctx, String dbNam
}
@Override
- public void onClose() {
+ public synchronized void onClose() {
+ ThreadPoolExecutor retiredExecutor = threadPoolWithPreAuth;
+ threadPoolWithPreAuth = null;
super.onClose();
- if (null != catalog) {
- try {
- if (catalog instanceof AutoCloseable) {
- ((AutoCloseable) catalog).close();
- }
- catalog = null;
- } catch (Exception e) {
- LOG.warn("Failed to close iceberg catalog: {}", getName(), e);
+ Catalog retiredCatalog = catalog;
+ catalog = null;
+ resourceTracker.retireCurrent(() -> {
+ closeCatalog(retiredCatalog);
+ if (retiredExecutor != null) {
+ ThreadPoolManager.shutdownExecutorService(retiredExecutor);
+ }
+ });
+ }
+
+ @Override
+ public synchronized void resetToUninitialized(boolean invalidCache) {
+ ExternalMetaCacheMgr cacheMgr =
Env.getCurrentEnv().getExtMetaCacheMgr();
+ resetCatalogRuntime(cacheMgr, invalidCache);
+ }
+
+ private void resetCatalogRuntime(ExternalMetaCacheMgr cacheMgr, boolean
invalidCache) {
+ cacheMgr.removeCatalogByEngine(getId(),
IcebergExternalMetaCache.ENGINE);
+ super.resetToUninitialized(invalidCache);
+ }
+
+ private void closeCatalog(Catalog retiredCatalog) {
+ if (retiredCatalog == null) {
+ return;
+ }
+ try {
+ if (retiredCatalog instanceof AutoCloseable) {
+ ((AutoCloseable) retiredCatalog).close();
Review Comment:
[P1] Close HadoopCatalog's shared FileIO
For `type=hadoop`, this invokes Iceberg 1.10.1 `HadoopCatalog.close()`, but
that method closes only its lock manager and metrics reporter; the catalog-wide
`fileIO` loaded during `initialize()` is not in its `CloseableGroup`. Table
eviction correctly does not close this shared IO, so repeated reset/ALTER/close
abandons one custom FileIO/client generation after the tracker drains. Give the
Hadoop catalog generation an idempotent owner for that exact FileIO and close
it here after active leases finish, with repeated-reset close-counting coverage.
--
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]
