github-actions[bot] commented on code in PR #66913: URL: https://github.com/apache/doris/pull/66913#discussion_r3869771618
########## fe/fe-core/src/main/java/org/apache/doris/datasource/hudi/HudiFsViewCacheValue.java: ########## @@ -0,0 +1,111 @@ +// 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.datasource.hudi; + +import org.apache.hudi.common.table.view.HoodieTableFileSystemView; + +/** + * Reference-counted wrapper around a shared {@link HoodieTableFileSystemView}. + * + * <p>The underlying fs view is cached per table and shared by concurrent scan nodes. Closing it while + * another thread is still planning splits is unsafe, so the cache only closes the view after the entry has + * been evicted AND all acquired references have been released. + */ +public class HudiFsViewCacheValue { + private final HoodieTableFileSystemView fsView; + // The loader owns one transferable reference until getFsView hands this exact generation to its first caller. + private int refCount = 1; + private boolean loaderReferenceAvailable = true; + private boolean evicted = false; + private boolean closed = false; + + public HudiFsViewCacheValue(HoodieTableFileSystemView fsView) { + this.fsView = fsView; + } + + public synchronized Lease tryAcquire() { + Lease lease; + if (loaderReferenceAvailable) { + loaderReferenceAvailable = false; + lease = new Lease(this, fsView); + } else if (evicted) { + return null; + } else { + refCount++; + lease = new Lease(this, fsView); + } + try { + // The cache uses expire-after-access without detached refresh. Sync every foreground generation handoff + // so a continuously hot key still observes newly completed commits. + fsView.sync(); Review Comment: [P1] Do not hold the generation-owner monitor across Hudi I/O The lease reference has already been transferred/incremented before this call, but `tryAcquire()` remains synchronized while `fsView.sync()` reloads the Hudi timeline and may perform storage reads. Synchronous eviction during HMS reset/ALTER calls `evict()` on the same monitor while holding the catalog monitor and lifecycle stripe, so a stalled timeline read blocks the entire reset even though the counted lease already makes concurrent retirement safe. Leave the ownership monitor after acquiring the reference, run `sync()` outside it, and close the lease on failure; a blocked-sync test should prove reset can mark the generation retired while final close still waits for that lease. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalMetaCache.java: ########## @@ -110,7 +114,12 @@ public IcebergExternalMetaCache(ExecutorService refreshExecutor, ExternalMetaCac this::loadTableCacheValue, defaultEntryCacheSpec(), MetaCacheEntryInvalidation.forNameMapping(nameMapping -> nameMapping)) .withSizeEstimator(this::prepareTableForCachePublication) - .withReplacementListener(this::retireTableGeneration)); + .withReplacementListener(this::retireTableGeneration) + .withRemovalListener(value -> value, (key, value) -> { Review Comment: [P1] Retire refresh values that never publish These listeners only see values that entered Caffeine. The generation-fenced refresh paths can load a new `IcebergTableCacheValue` and then abandon it: non-weighted refresh returns if invalidation/close changes the mutation token after load, while weighted refresh ignores `DISABLED`, `NOT_CURRENT`, or `REJECTED` from admission. That internal value is never inserted or returned, but it still owns both loader/cache references plus the table-FileIO and catalog-generation cleanup, so no borrower or removal callback can call `retire()`. Explicitly retire an unpublished refresh value unless publication transfers ownership, and test post-load invalidation and weighted rejection with a resource-bearing value. -- 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]
