API: Introduce rollbackTransaction() API

[deniskuzZ]

#15377

[dramaticlly]

I think we might need to add a default method for this new method to avoid break existing class that implemented the Transaction interface.

[nastra]

yeah just having a default impl that throws an UOE should be enough here. See also https://iceberg.apache.org/contribute/#adding-a-default-implementation

[pvary]

Could it be isAbortedOrCommitted? Or is Finalized?

We lso need to prevent an abort when the Transaction was committed.

[steveloughran]

I'd make it a state enum with an atomic reference, then a checkTransactionState(TransactionState) before nearly all calls.

[deniskuzZ]

Done, added State enum.

Also had to add CommitMode enum to support external commit coordination (e.g., multi-table transactions).
For multi-table transactions, the plan is to use StagingTableOperations, which overrides doCommit and operates in EXTERNAL commit mode.

[deniskuzZ]

public class StagingTableOperations extends HiveTableOperations{

    private String newMetadataLocation;

    public StagingTableOperations(
            Configuration conf,
            ClientPool<IMetaStoreClient, TException> metaClients,
            KeyManagementClient keyManagementClient,
            FileIO fileIO,
            String catalogName,
            String database,
            String table) {
        super(conf, metaClients, fileIO, keyManagementClient, catalogName, database, table);
    }

    @Override
    protected void doCommit(TableMetadata base, TableMetadata metadata) {
        newMetadataLocation = writeNewMetadataIfRequired(base == null, metadata);
    }

    @Override
    public CommitMode commitMode() {
        return CommitMode.EXTERNAL;
    }

    public String metadataLocation() {
        return newMetadataLocation;
    }
}

---

public class HiveTransaction extends BaseTransaction {
   ...
  public HiveTransaction(Table table, HiveTableOperations ops) {
    this(table, ops, ops.toStagingOps());
  }

  private HiveTransaction(Table table, HiveTableOperations ops) {
      super(table.name(), stagingOps, TransactionType.SIMPLE, ops.current());
  }
}

[pvary]

What is the order of the calls during the Hive commit?

1. Stage every change (is this calling the original commit, but the ops are changes so they not really commit, just prepare?)
2. Lock every table
3. Check if no change happened to the tables
4. Update every table metadata (calling a real commit on all the ops)
5. Release locks

?

I'm not big fan of the change in TableOperations.

Maybe remove the state checks in BaseTransaction, or expose them on the BaseTransaction like BaseTransaction.checkState(State from, State to).

I prefer to remove the checks at this point. Any better ideas @RussellSpitzer?

[deniskuzZ]

Multi-table commit:

table loop {

1. stage every change: BaseTransaction#commit -> StagingTableOperations#doCommit, writes metadata file only, skipping the locking
2. lock if enabled (HMSLock)
3. verify whether Base metadata location is same as the current table metadata location

}
4. table properties batch commit using CAS (NoLock)
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception

Currently, we rely on reflection to invoke BaseTransaction#cleanUpOnCommitFailure, and addressing that limitation is the purpose of this PR.

---

As discussed, removed the guards in 2nd commit.

I think having guards in place makes sense to avoid ending up in an inconsistent state. Calling rollback after a commit is destructive, as it would remove already committed data.

On the other hand, adding such guards would require introducing commitMode/isStaging flag (see the 1st commit).

[qlong]

Can we add prepareMetadata() method to HiveTableOperations like this? It does step 1 of your multi-table commit flow, so no need to call commitTransaction:

public String prepareMetadata(TableMetadata metadata) {
    // ... some setup 
    this.stagedMetadataLocation = writeNewMetadataIfRequired(false, metadata);
    return stagedMetadataLocation;
}

change doCommit to skip writeNewMetadataIfRequired if this.stagedMetadataLocation != null.

With this change:

• there will be no rollbackTransaction after commitTransaction -- correct transaction semantics is restored
• the state guard can be added back cleanly, which is important as rollback is now a public api

[deniskuzZ]

@qlong, commitTransaction aggregates all table updates into a single table operation and then commits it — see https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L373-L375.

applyUpdates() is private, and we also need to perform cleanup after a successful execution: https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/BaseTransaction.java#L394-L416

StagingTableOperations passed to BaseTransaction already behaves as described, so there’s no need for an additional prepareMetadata API.

What’s missing for multi-table commits is the cleanAll functionality, which is currently private. Making it package-private would also be sufficient.

[steveloughran]

Extra tests:

• Cannot add changed to a txn after abort or commit
• dual commit forbidden
• abort after commit forbidden
• dual abort ok, but second attempt a no-op.

Actually,given the subclassing, forbidding dual abort is the way to guarantee subclasses don't do things on a second abort call

[deniskuzZ]

Extra tests:

• Cannot add changed to a txn after abort or commit
• dual commit forbidden
• abort after commit forbidden
• dual abort ok, but second attempt a no-op.

Actually,given the subclassing, forbidding dual abort is the way to guarantee subclasses don't do things on a second abort call

Tnx, added tests.
@steveloughran, FYI subclassing is restricted by the package-private constructor, but you are right