github-actions[bot] commented on code in PR #66913:
URL: https://github.com/apache/doris/pull/66913#discussion_r3903084864
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergTransaction.java:
##########
@@ -148,7 +150,8 @@ public void beginInsert(ExternalTable dorisTable, Table
targetTable,
this.writeSchemaContext = insertCtx == null
? Optional.empty() : insertCtx.getWriteSchemaContext();
try {
- ops.getExecutionAuthenticator().execute(() -> {
+ acquireWritableTableLease(dorisTable);
Review Comment:
[P1] Roll back the registered transaction when begin setup fails
`beginTransaction()` has already registered this transaction before
`executeSingleInsert()` invokes `beforeExec()`, but `beforeExec()` is outside
its `try`/`onFail()` scope. This new expected-generation acquisition can throw
after a reset (and branch/schema/format validation can throw too); the local
catch releases any partial lease, then exits without
`transactionManager.rollback(txnId)`, leaving both the manager-local and global
registry entries behind. Put `beforeExec()` inside the failure/rollback scope
for normal and empty inserts, or explicitly roll back the captured manager for
every post-registration begin failure, and assert both registries are empty in
those regressions.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergTransaction.java:
##########
@@ -770,17 +827,21 @@ public boolean requireStrictCleanup() {
@Override
public void rollback() {
- if (isRewriteMode) {
- // Clear the collected files for rewrite mode
- synchronized (filesToDelete) {
- filesToDelete.clear();
- }
- synchronized (filesToAdd) {
- filesToAdd.clear();
+ try {
+ if (isRewriteMode) {
+ // Clear the collected files for rewrite mode
+ synchronized (filesToDelete) {
+ filesToDelete.clear();
+ }
+ synchronized (filesToAdd) {
+ filesToAdd.clear();
+ }
+ LOG.info("Rewrite transaction rolled back");
}
- LOG.info("Rewrite transaction rolled back");
+ // For insert mode, do nothing as original implementation
+ } finally {
+ releaseWritableTableLease();
Review Comment:
[P1] Do not gate this rollback on the live catalog authenticator
Ordinary DML reaches this failure release through
`BaseExternalTableInsertExecutor.onFail()`, which first fetches
`externalTable.getCatalog().getExecutionAuthenticator()`. ALTER/reset nulls
that mutable field while this G1 lease intentionally keeps the retired runtime
alive; `onFail()` catches the lookup failure and never invokes the captured
`transactionManager.rollback(txnId)`. The manager/global entries and the G1
catalog/FileIO/executor then remain pinned. Invoke the captured manager
unconditionally for local rollback (or use the retained generation's
authenticator), and cover an execution failure after reset.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergExternalMetaCache.java:
##########
@@ -544,11 +1047,14 @@ private IcebergSnapshotCacheValue loadSnapshotProjection(
MTMVRelatedTableIf table = (MTMVRelatedTableIf) dorisTable;
IcebergSnapshot latestIcebergSnapshot =
IcebergUtils.getLatestIcebergSnapshot(projectionTable);
IcebergPartitionInfo icebergPartitionInfo;
- if (!table.isValidRelatedTable()) {
+ boolean validRelatedTable = validRelatedTableOverride == null
+ ? table.isValidRelatedTable() : validRelatedTableOverride;
Review Comment:
[P1] Classify the same table generation being projected
Without a `StatementContext`, `getSnapshotCache()` already owns a bounded G1
operation lease and passes its G1 `projectionTable` here. This fallback calls
`IcebergExternalTable.isValidRelatedTable()`, which opens a second independent
`withIcebergTable()` borrow; refresh/reset can make that G2, so G2
partition-spec eligibility decides whether G1 partition metadata is loaded and
caches that answer. Background dictionary/MTMV freshness can then see an empty
projection or report 0 for the G1 snapshot. Compute the override from the
already-resolved `projectionTable`, as the writable-lease overload does, and
add a no-context reset barrier regression.
--
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]
