morningman commented on code in PR #68902:
URL: https://github.com/apache/doris/pull/68902#discussion_r4240633910


##########
regression-test/suites/external_table_p0/kerberos/test_iceberg_hadoop_catalog_kerberos.groovy:
##########
@@ -97,6 +97,72 @@ suite("test_iceberg_hadoop_catalog_kerberos", "p0,external") 
{
           'partition_val2'
         );
     """
+    def snapshotsAfterFirstInsert = sql """
+        select snapshot_id from ${test_tbl_name}\$snapshots order by 
committed_at
+    """
+    assert snapshotsAfterFirstInsert.size() == 1
+    String firstSnapshotId = String.valueOf(snapshotsAfterFirstInsert[0][0])
+
+    sql """
+        insert into ${test_tbl_name} values (
+          '2024-05-27 12:34:56',
+          false,
+          456,
+          4567890123456,
+          45.67,
+          89.012,
+          45678.9012,
+          'another example',
+          '2024-05-27',
+          '2024-05-27 14:00:00',
+          'partition_val1',
+          'partition_val2'
+        );
+    """
+    def dataBeforeRollback = sql """select count(1) from ${test_tbl_name}"""
+    assert dataBeforeRollback.get(0).get(0) == 2
+
+    // Rollback must commit metadata with the same Kerberos identity used to 
acquire the table.
+    sql """
+        alter table ${catalog_name}.${database_name}.${test_tbl_name}
+        execute rollback_to_snapshot("snapshot_id" = "${firstSnapshotId}")
+    """
+    def dataAfterRollback = sql """select count(1) from ${test_tbl_name}"""
+    assert dataAfterRollback.get(0).get(0) == 1
+
+    sql """
+        insert into ${test_tbl_name} values (
+          '2024-05-27 12:34:56',
+          false,
+          456,
+          4567890123456,
+          45.67,
+          89.012,
+          45678.9012,
+          'another example',
+          '2024-05-27',
+          '2024-05-27 14:00:00',
+          'partition_val1',
+          'partition_val2'
+        );
+    """
+
+    def snapshotsBeforeExpire = sql """
+        select snapshot_id from ${test_tbl_name}\$snapshots order by 
committed_at
+    """
+    assert snapshotsBeforeExpire.size() == 3
+
+    // The action must reuse the catalog's Kerberos identity while committing 
new metadata.
+    sql """
+        alter table ${catalog_name}.${database_name}.${test_tbl_name}
+        execute expire_snapshots("retain_last" = "1")

Review Comment:
   Agreed, and CI confirmed it: the External run on 6e7dff9a675 failed this 
exact step with `Failed to expire snapshots: Failed to open input stream for 
file: hdfs://.../metadata/snap-*.avro`.
   
   Fixed in 8275ce3e768. `IcebergProcedureOps` now hands the procedure body and 
the rewrite planner a table whose `FileIO` opens, lists and deletes every file 
inside the plugin `doAs`, the same wrap the scan path (`wrapTableForScan`) and 
the write path (`openTransaction`) already use. A file opened that way keeps 
its authenticated connection on any thread, so the manifest lists 
`RemoveSnapshots` reads on the worker pool after the commit are covered. The 
wrapped `FileIO` keeps `HadoopFileIO`'s prefix listing, which 
`remove_orphan_files` requires, and hides bulk deletes, so deletes go through 
the authenticated per-file `deleteFile` rather than `HadoopFileIO`'s own delete 
pool. The same seam also covers `cherrypick_snapshot` / `publish_changes` (the 
new manifest is written on the worker pool) and `rewrite_data_files` planning 
on a multi-manifest table.
   
   `IcebergProcedureOpsKerberosIoTest` reproduces your sequence (rollback, then 
`expire_snapshots(retain_last = 1)`) on a local Hadoop table, with an 
authenticator that marks only the thread inside its `doAs`. Without the wrap it 
records the six worker-pool opens of the expired manifest lists and manifests 
outside the `doAs`.
   
   On keeping the refresh when cleanup fails after the commit: not changed 
here. `ConnectorExecuteAction` refreshes only when the procedure returns, and 
branch-4.x's `ExecuteActionCommand` behaves the same way (`logRefreshTable` 
runs only after the action returns), so this PR does not regress it. With the 
wrap, the Kerberos case no longer throws after the commit; a post-commit 
cleanup failure for any other reason would still skip the refresh, which is a 
separate change.
   



-- 
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]

Reply via email to