github-actions[bot] commented on code in PR #66914:
URL: https://github.com/apache/doris/pull/66914#discussion_r3829408055
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnector.java:
##########
@@ -1525,24 +1568,31 @@ public java.util.logging.Logger getParentLogger()
throws java.sql.SQLFeatureNotS
@Override
public void close() throws IOException {
+ // Release cache-owner references first. The resource tracker keeps
the catalog generation alive until
+ // every table owner that was loaded through it has also released its
last statement borrower.
+ invalidateAll();
Review Comment:
[P1] Fence new publications before taking the teardown snapshot. This
invalidation runs before the tracker is closed, so a cache miss admitted in
between can finish after `close()`, promote its pre-close guard, and publish a
`TableOwner` into the already-cleared cache; its cache reference then pins the
retired catalog generation forever. The same gap exists in
`getOrCreateCatalog()`: a slow first build can see null, let this method
snapshot null, and assign a fresh catalog after the fields are cleared, leaving
that client unclosed and unusable by the closed tracker. This is distinct from
the existing active-borrower and rejected-REST-replacement threads because
those resources were already published or went through `rotate()`. Please
coordinate close with catalog/cache-load admission, retire or directly close
every late publication, and add latch tests for both interleavings.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergStatementScope.java:
##########
@@ -83,13 +83,97 @@ static Table sharedTable(ConnectorSession session, String
dbName, String tableNa
() -> snapshotReadTable(loader.get()));
}
+ /**
+ * Statement-scoped variant for a table borrowed from {@link
IcebergTableCache}. The memoized holder is
+ * {@link AutoCloseable}, so the engine's statement-scope teardown
releases the borrower only after scan
+ * pumps have quiesced. Cache eviction and statement completion may happen
in either order; the underlying
+ * FileIO is closed only after both owners release it.
+ */
+ static Table sharedBorrowedTable(ConnectorSession session, String dbName,
String tableName,
+ Supplier<IcebergTableCache.TableLease> loader, Supplier<Table>
unscopedLoader) {
+ if (session == null || session.getStatementScope() ==
ConnectorStatementScope.NONE) {
+ // NONE has no statement-end callback, so it cannot safely own a
lease. Preserve its original direct
+ // load-every-time behavior; creating a lease here would drop its
only close handle and leak forever.
+ return snapshotReadTable(unscopedLoader.get());
+ }
+ ScopedBorrow borrowed = ConnectorStatementScopes.resolveInStatement(
+ session, TABLE_NAMESPACE, dbName, tableName, () -> new
ScopedBorrow(loader.get()));
+ return borrowed.table;
+ }
+
+ /** Statement-owned direct table for credential-dependent catalogs where
cross-query caching is disabled. */
+ static Table sharedTrackedTable(ConnectorSession session, String dbName,
String tableName,
+ IcebergCatalogResourceTracker resourceTracker, Supplier<Table>
loader) {
+ if (session == null || session.getStatementScope() ==
ConnectorStatementScope.NONE) {
+ return snapshotReadTable(loader.get());
+ }
+ TrackedTable tracked = ConnectorStatementScopes.resolveInStatement(
+ session, TABLE_NAMESPACE, dbName, tableName,
+ () -> new TrackedTable(resourceTracker.load(loader), true));
+ return tracked.table();
+ }
+
+ private static final class ScopedBorrow implements AutoCloseable {
+ private final IcebergTableCache.TableLease lease;
+ private final Table table;
+
+ private ScopedBorrow(IcebergTableCache.TableLease lease) {
+ this.lease = lease;
+ this.table = snapshotReadTable(lease.table());
+ }
+
+ @Override
+ public void close() {
+ lease.close();
+ }
+ }
+
/** Loads the mutable table used only by write planning and transaction
creation. */
static Table sharedWritableTable(
ConnectorSession session, String dbName, String tableName,
Supplier<Table> loader) {
return ConnectorStatementScopes.resolveInStatement(
session, WRITABLE_TABLE_NAMESPACE, dbName, tableName, loader);
}
+ /** Mutable table paired with the exact catalog generation that produced
it. */
+ static TrackedTable sharedTrackedWritableTable(ConnectorSession session,
String dbName, String tableName,
+ IcebergCatalogResourceTracker resourceTracker, Supplier<Table>
loader) {
+ if (session == null || session.getStatementScope() ==
ConnectorStatementScope.NONE) {
+ return new TrackedTable(resourceTracker.load(loader), false);
+ }
+ return ConnectorStatementScopes.resolveInStatement(
+ session, WRITABLE_TABLE_NAMESPACE, dbName, tableName,
+ () -> new TrackedTable(resourceTracker.load(loader), true));
+ }
+
+ static final class TrackedTable implements AutoCloseable {
+ private final IcebergCatalogResourceTracker.TrackedResource<Table>
tracked;
+ private final boolean statementOwned;
+
+ private
TrackedTable(IcebergCatalogResourceTracker.TrackedResource<Table> tracked,
+ boolean statementOwned) {
+ this.tracked = tracked;
+ this.statementOwned = statementOwned;
+ }
+
+ Table table() {
+ return tracked.resource();
+ }
+
+ IcebergCatalogResourceTracker.ResourceLease retainLease() {
+ return tracked.retainLease();
+ }
+
+ boolean isStatementOwned() {
+ return statementOwned;
+ }
+
+ @Override
+ public void close() {
+ tracked.close();
Review Comment:
[P1] Retire the table-owned FileIO with the direct table owner.
`TrackedTable.close()` releases only the catalog-generation lease, while the
flavor-aware table cleanup is wired only into `IcebergTableCache`, which these
direct paths bypass. Consequently Glue/S3Tables writes drop their independently
owned per-table IO after commit/rollback without closing it; REST
vended-credential reads have the same problem because the cache is disabled and
Iceberg creates a separate table IO for response credentials, tracked only by a
weak-key tracker until GC or catalog close. The existing threads cover keeping
the catalog generation alive while operations are active, not releasing the
table-owned resource after the final consumer. Please make the direct owner
combine/ref-count both lifetimes through statement and transaction completion,
and add direct-read plus commit/rollback cleanup tests.
--
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]
