[impala] 01/02: IMPALA-11508: Deflake test_expire_snapshots

[bo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

boroknagyz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit aaf6fdc645b2f67e67f009c8e6dcf3ce25b22c98
Author: Zoltan Borok-Nagy <bo...@cloudera.com>
AuthorDate: Fri Sep 23 16:35:47 2022 +0200

    IMPALA-11508: Deflake test_expire_snapshots
    
    Before this patch test_expire_snapshots failed frequently. The patch is
    only a few lines of code, but there are some subtleties here:
    
    IcebergCatalogOpExecutor.alterTableExecute() didn't use Iceberg
    transactions to carry out the operation. This means that
    expireSnapshots() resulted in an ALTER TABLE operation on its own
    which we also got during event processing.
    
    Because this ALTER TABLE event didn't had the catalog version set we
    didn't recognized it as a self-event. This caused unnecessary table
    reloads during the tests which manifested in
    InconsistentMetadataFetchException: "... table ... changed version
    between accesses" errors.
    
    With this patch IcebergCatalogOpExecutor.alterTableExecute() takes
    an Iceberg transaction object and invokes expireSnapshots() in the
    context of this Iceberg transaction. This Iceberg transaction also
    sets table properties "impala.events.catalogServiceId" and
    "impala.events.catalogVersion". And because everything happens in
    a single Iceberg transaction we only create a single ALTER TABLE
    which we can recognize during event processing (based on the
    table properties), avoiding unnecessary table reloads.
    
    Testing:
     * executed test_expire_snapshots in a loop
    
    Change-Id: I6d82c8b52466a24af096fe5fe4dbd034a1ee6a15
    Reviewed-on: http://gerrit.cloudera.org:8080/19036
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java     | 2 +-
 .../main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
index 4771375f4..6a9e833e1 100644
--- a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
+++ b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
@@ -1326,7 +1326,7 @@ public class CatalogOpExecutor {
           break;
         case EXECUTE:
           Preconditions.checkState(params.isSetSet_execute_params());
-          String summary = IcebergCatalogOpExecutor.alterTableExecute(tbl,
+          String summary = IcebergCatalogOpExecutor.alterTableExecute(iceTxn,
               params.getSet_execute_params());
           addSummary(response, summary);
           break;
diff --git a/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java b/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
index 2a482e2bb..75aa6e9a0 100644
--- a/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
+++ b/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java
@@ -177,9 +177,9 @@ public class IcebergCatalogOpExecutor {
     tableOp.commit(metadata, newMetadata);
   }
 
-  public static String alterTableExecute(FeIcebergTable tbl,
+  public static String alterTableExecute(Transaction txn,
       TAlterTableExecuteParams params) {
-    ExpireSnapshots expireApi = tbl.getIcebergApiTable().expireSnapshots();
+    ExpireSnapshots expireApi = txn.expireSnapshots();
     expireApi.expireOlderThan(params.older_than_millis);
     expireApi.commit();
     return "Snapshots have been expired.";