github-actions[bot] commented on code in PR #66914:
URL: https://github.com/apache/doris/pull/66914#discussion_r3830345376
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorTransaction.java:
##########
@@ -1335,7 +1369,11 @@ public void rollback() {
@Override
public void close() {
- // No resources to release: the SDK transaction holds no connections
of its own.
+ IcebergStatementScope.TrackedTableLease lease = tableLease;
Review Comment:
[P1] Seal beginWrite before releasing transaction ownership. `close()` is
not linearized with `beginWrite()`. On a distributed rewrite timeout,
`runGroups()` returns without joining its worker tasks and the driver
immediately rolls back/removes this shared transaction. If a worker is still
blocked in the remote table load, this read can observe `tableLease == null`;
the worker can then resume and publish its retained lease after the only close,
permanently pinning the table-owned FileIO and catalog generation. Please
seal/close under `beginLock` (or equivalent terminal state), reject late
begins, and locally release any lease whose publication loses the race; a latch
test for timeout/rollback during table load would cover it.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -1063,6 +1063,23 @@ public boolean isDeferredForArrowFlight() {
return deferredForArrowFlight;
}
+ void deferArrowFlightQuery() {
+ deferredForArrowFlight = true;
+ boolean registered;
+ try {
+ registered = context.addFlightSqlDeferredExecutor(this);
+ } catch (RuntimeException | Error t) {
+ deferredForArrowFlight = false;
+ throw t;
+ }
+ if (!registered) {
+ // Session teardown sealed and drained the registry before this
registration. Finalize directly:
+ // no later owner can reach this executor, and the deferred flag
prevents the ordinary finally block
+ // from closing the same coordinator a second time.
+ finalizeArrowFlightQuery();
Review Comment:
[P1] Fail GetFlightInfo when teardown finalizes this query. Session teardown
can win here while this `GetFlightInfo` is still executing. This branch
finalizes the coordinator (including the external batch `SplitSource`) but
returns normally, so the producer can continue to build and return a successful
ticket; the add-before-seal ordering has the same problem when the teardown
drain closes the registered executor before `FlightInfo` is published. The
later `DoGet` then receives a ticket for already-released query resources.
Please fence teardown against the in-flight producer result or surface
cancellation after finalizing, and add a producer-level race test for both
orderings.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/ReauthenticatingRestSessionCatalog.java:
##########
@@ -124,9 +134,32 @@ private synchronized void
reauthenticate(RESTSessionCatalog attemptedOn, Runtime
+ "then retrying the request once.", name(), cause);
RESTSessionCatalog replacement = delegateBuilder.get();
RESTSessionCatalog wedged = delegate;
- delegate = replacement;
+ if (resourceTracker == null) {
+ delegate = replacement;
+ closeReplacedDelegate(wedged);
+ return;
+ }
+ try {
+ invalidateTables.run();
Review Comment:
[P1] Invalidate after publishing the replacement generation. The cache
remains open during reauthentication, so a miss can start after this sole
flush, capture the new cache generation, and load against the still-current
delegate while `rotate()` publishes the replacement. Because no invalidation
follows rotation, that miss may publish a `TableOwner` holding the retired
generation; an expire-after-access hot entry can then keep the wedged
auth/FileIO client graph alive indefinitely, and repeated 401 windows can
accumulate them. Please order publication before invalidation or atomically
couple cache admission to delegate generation, with a latch test for a miss
admitted in this window.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergWritePlanProvider.java:
##########
@@ -1054,7 +1063,13 @@ private Table resolveTable(ConnectorSession session,
IcebergTableHandle handle)
throw new DorisConnectorException("Failed to load iceberg
table "
+ handle.getDbName() + "." + handle.getTableName() +
": " + e.getMessage(), e);
}
- });
+ };
+ return resourceTracker == null
+ ? IcebergStatementScope.sharedWritableTable(
+ session, handle.getDbName(), handle.getTableName(),
loader)
+ : IcebergStatementScope.sharedTrackedWritableTable(
Review Comment:
[P1] Close the rewrite task scope when sink planning fails. This branch adds
a statement-owned `TrackedTable`, but `ConnectorRewriteGroupTask` can fail
later in `initPlan` (for example, a post-`beginWrite` bound-metadata/column
validation) before it enters `executeSingleInsert`. Its catch/finally neither
unregisters the task query nor closes the fresh `StatementContext`, so the
registered query-finish callback never runs; outer rollback releases only the
transaction lease, while this scope owner permanently pins the table cleanup
and catalog generation. Please give the task a terminal cleanup for every exit
and add a post-begin sink-plan failure test that proves the callback/scope is
drained.
--
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]
