github-actions[bot] commented on code in PR #66914:
URL: https://github.com/apache/doris/pull/66914#discussion_r3820887675
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnector.java:
##########
@@ -1525,6 +1581,10 @@ public java.util.logging.Logger getParentLogger() throws
java.sql.SQLFeatureNotS
@Override
public void close() throws IOException {
+ // Release connector-owned cache references while the catalog objects
are still available to classify
+ // and close per-table FileIO. Active statement borrowers retain their
own references and defer the actual
+ // cleanup until their leases close.
+ invalidateAll();
Review Comment:
[P1] Defer catalog teardown until active table leases finish.
invalidateAll() releases only Doris's cache reference, but the following
Glue/REST/S3Tables catalog close directly closes Iceberg's FileIOTracker or
shared IO; those upstream owners are outside TableOwner.references, so an
active ScopedBorrow is still poisoned despite the comment above. REST 401
reauthentication similarly closes the old delegate without invalidating or
generation-keying cached tables. Please make leases retain the catalog/delegate
generation and retire it after its final borrower, then test connector close
and 401 replacement with a real active lease.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergStatementScope.java:
##########
@@ -83,6 +83,39 @@ 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) {
Review Comment:
[P1] Do not bypass the enabled table cache for NONE sessions.
PluginDrivenExternalTable.initSchema, row-count, and statistics loaders all use
buildCrossStatementSession(), which forces NONE, so this production branch now
calls the raw catalog loader even when the pre-PR positive-TTL cache exists.
For Glue/S3Tables each load owns a new tracked FileIO; because this path has
neither a lease nor an explicit close, cleanup falls back to Iceberg's weak-key
tracker/GC and recreates the nondeterministic client retention this PR is meant
to fix. Please preserve cache reuse and make the complete cross-statement
operation own/close a lease (or equivalent), with production-shaped tests.
##########
fe/fe-connector/fe-connector-hudi/src/main/java/org/apache/doris/connector/hudi/HudiScanPlanProvider.java:
##########
@@ -294,30 +294,33 @@ basePath, inputFormat, serdeLib, columnNames,
columnTypes, partitionFieldNames(m
HoodieLocalEngineContext engineCtx = new
HoodieLocalEngineContext(metaClient.getStorageConf());
HoodieTableFileSystemView fsView =
FileSystemViewManager.createInMemoryFileSystemView(
engineCtx, metaClient, metadataConfig);
+ try {
+ // Resolve partitions
+ List<String> partitionPaths = resolvePartitions(hudiHandle,
metaClient);
- // Resolve partitions
- List<String> partitionPaths = resolvePartitions(hudiHandle,
metaClient);
-
- List<ConnectorScanRange> ranges = new ArrayList<>();
- for (String partitionPath : partitionPaths) {
- Map<String, String> partValues = parsePartitionValues(
- partitionPath, hudiHandle.getPartitionKeyNames());
+ List<ConnectorScanRange> ranges = new ArrayList<>();
+ for (String partitionPath : partitionPaths) {
+ Map<String, String> partValues = parsePartitionValues(
+ partitionPath, hudiHandle.getPartitionKeyNames());
- if (useNativeCowPath) {
- collectCowSplits(fsView, partitionPath, queryInstant,
- basePath, partValues, ranges, schemaIdResolver);
- } else {
- collectMorSplits(fsView, partitionPath, queryInstant,
- basePath, inputFormat, serdeLib,
- columnNames, columnTypes, partValues, forceJni,
ranges, schemaIdResolver);
+ if (useNativeCowPath) {
+ collectCowSplits(fsView, partitionPath, queryInstant,
+ basePath, partValues, ranges, schemaIdResolver);
+ } else {
+ collectMorSplits(fsView, partitionPath, queryInstant,
+ basePath, inputFormat, serdeLib,
+ columnNames, columnTypes, partValues, forceJni,
ranges, schemaIdResolver);
+ }
}
- }
- LOG.info("Hudi scan planning: {}.{} type={} partitions={} splits={}",
- hudiHandle.getDbName(), hudiHandle.getTableName(),
- hudiHandle.getHudiTableType(), partitionPaths.size(),
ranges.size());
+ LOG.info("Hudi scan planning: {}.{} type={} partitions={}
splits={}",
+ hudiHandle.getDbName(), hudiHandle.getTableName(),
+ hudiHandle.getHudiTableType(), partitionPaths.size(),
ranges.size());
- return ranges;
+ return ranges;
+ } finally {
+ fsView.close();
Review Comment:
[P2] Preserve the planning failure if cleanup also fails. Hudi 1.0.2's
AbstractTableFileSystemView.close() wraps tableMetadata.close() failures in
HoodieException, while this raw finally replaces any exception already thrown
by partition/split planning; MORIncrementalRelation has the same pattern.
Please use try-with-resources (or addSuppressed explicitly) for both views and
cover success plus body-and-close failure paths. The new lifecycle test only
exercises listAllPartitionPaths, so deleting either fsView.close still leaves
it green.
##########
fe/fe-connector/fe-connector-hudi/pom.xml:
##########
@@ -258,6 +258,16 @@ under the License.
<artifactId>junit-jupiter</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.mockito</groupId>
+ <artifactId>mockito-core</artifactId>
Review Comment:
[P2] Keep connector tests free of Mockito. fe/fe-connector/AGENTS.md
explicitly requires recording fakes and says connector POMs carry no mocking
framework. This single HoodieTableMetadata.close() assertion introduces
mockito-core and the additional Kryo test-classpath closure it needs. Please
replace the mock with a recording fake or narrow lifecycle seam and remove both
dependencies.
--
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]
