github-actions[bot] commented on code in PR #67695:
URL: https://github.com/apache/doris/pull/67695#discussion_r3967906432
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/action/IcebergRewriteDataFilesAction.java:
##########
@@ -75,8 +76,8 @@ public class IcebergRewriteDataFilesAction extends
BaseIcebergAction {
public IcebergRewriteDataFilesAction(Map<String, String> properties,
Optional<PartitionNamesInfo> partitionNamesInfo,
- Optional<Expression> whereCondition) {
- super("rewrite_data_files", properties, partitionNamesInfo,
whereCondition);
+ Optional<Expression> whereCondition, IcebergMetadataOps
metadataOps) {
+ super("rewrite_data_files", properties, partitionNamesInfo,
whereCondition, metadataOps);
Review Comment:
[P1] Keep `rewrite_data_files` on the captured catalog generation. This
constructor now accepts `metadataOps`, but `executeAction` still plans from the
unfenced cached `IcebergUtils.getIcebergTable(...)`; `RewriteDataFileExecutor`
then re-reads the catalog's current transaction manager and loads a separate
MVCC snapshot. During a catalog property reset, the manager is cleared before
the old Iceberg cache generation is retired, so this path can plan generation-A
`FileScanTask`/`DataFile` objects and then fail on a cleared manager or
register them against generation B. Its broad `catch (Exception)` also turns a
generation-movement failure into `UserException`, so the command's new safe
retry never runs. Please carry one captured ops/table/transaction context
through planning and commit, and add a reset test for this action. This is
distinct from the existing scan-worker thread, which covers UGI propagation
rather than A/B transaction state.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ExecuteActionCommand.java:
##########
@@ -144,6 +140,54 @@ public Optional<Expression> getWhereCondition() {
return whereCondition;
}
+ private ResultSet executeWithCatalogGenerationRetry(ConnectContext ctx,
TableIf table) throws Exception {
+ ExternalTable externalTable = (ExternalTable) table;
+ ExternalCatalog catalog = externalTable.getCatalog();
+ for (int retry = 0; ; retry++) {
+ try {
+ ExecuteAction action;
+ ExecutionAuthenticator authenticator;
+ synchronized (catalog) {
+ // Reset also holds this monitor, so the action's metadata
ops and
+ // authenticator form one generation.
+ catalog.makeSureInitialized();
+ action = ExecuteActionFactory.createAction(
+ actionName, properties, partitionNamesInfo,
whereCondition, table);
+ authenticator = catalog.getExecutionAuthenticator();
+ }
+ if (!action.isSupported(table)) {
+ throw new AnalysisException("Action '" + actionName + "'
is not supported for this table engine");
+ }
+ action.validate(tableNameInfo, ctx.getCurrentUserIdentity());
+ return executeAuthenticated(action, externalTable,
authenticator);
+ } catch (CatalogGenerationChangedException e) {
+ // A generation fence fails before mutation, so rebuilding the
action is safe;
+ // never retry commit errors.
+ if (retry >= MAX_CATALOG_GENERATION_RETRIES) {
+ throw new UserException(e.getMessage(), e);
+ }
+ }
+ }
+ }
+
+ private ResultSet executeAuthenticated(ExecuteAction action, ExternalTable
table,
+ ExecutionAuthenticator authenticator) throws Exception {
+ try {
+ // Iceberg tables retain filesystem configuration, not the
caller's UGI, so loading the table and
+ // committing its metadata must stay within one catalog
authentication scope.
+ return authenticator.execute(() -> {
Review Comment:
[P1] Keep `rewrite_data_files`' commit-time manifest writes inside the
catalog authentication context. In Iceberg 1.10.1, adding replacement files
reaches `MergingSnapshotProducer.newDataFilesAsManifests()` and
`SnapshotProducer.writeManifests()`, which hard-codes
`ThreadPools.getWorkerPool()` rather than the pool configured by
`scanManifestsWith(...)`. Those global workers call the table `FileIO` to
create manifest files, so the surrounding caller-thread `doAs` cannot supply
this catalog's UGI and a Kerberized rewrite can finish its data tasks only to
fail during commit. Please route this manifest-write phase through the
catalog's pre-authenticated executor/context (or an Iceberg API/version that
makes the pool configurable) and add a default-HadoopFileIO Kerberos rewrite
that produces and commits replacement files. This is distinct from the existing
thread about the initial `planFiles()` scan; it is a later, separately
hard-coded executor.
--
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]
