You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "deniskuzZ (via GitHub)" <gi...@apache.org> on 2026/02/20 22:39:44 UTC
[PR] [API] Introduce an abortTransaction() API [iceberg]
deniskuzZ opened a new pull request, #15389:
URL: https://github.com/apache/iceberg/pull/15389
(no comment)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
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` flag (see the [1st commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840106304
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
````
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());
}
}
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840026221
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -252,25 +259,20 @@ UpdateSnapshotReferencesOperation updateSnapshotReferencesOperation() {
@Override
public void commitTransaction() {
+ Preconditions.checkState(state == State.OPEN, "Cannot commit transaction in state: %s", state);
+
Preconditions.checkState(
hasLastOpCommitted, "Cannot commit transaction: last operation has not committed");
switch (type) {
- case CREATE_TABLE:
- commitCreateTransaction();
- break;
-
- case REPLACE_TABLE:
- commitReplaceTransaction(false);
- break;
-
- case CREATE_OR_REPLACE_TABLE:
- commitReplaceTransaction(true);
- break;
+ case CREATE_TABLE -> commitCreateTransaction();
+ case REPLACE_TABLE -> commitReplaceTransaction(false);
+ case CREATE_OR_REPLACE_TABLE -> commitReplaceTransaction(true);
+ case SIMPLE -> commitSimpleTransaction();
+ }
- case SIMPLE:
- commitSimpleTransaction();
- break;
+ if (ops.commitMode() == CommitMode.AUTO) {
+ state = State.COMMITTED;
Review Comment:
I'm not sure about this one. Could you please help me understand?
Isn't it something the extended classes should handle?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840106304
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
````
public class StagingTableOperations extends HiveTableOperations {
private String newMetadataLocation;
public StagingTableOperations(
Configuration conf,
ClientPool<IMetaStoreClient, TException> metaClients,
FileIO fileIO,
String catalogName,
String database,
String table) {
super(conf, metaClients, fileIO, catalogName, database, table);
}
@Override
protected void doCommit(TableMetadata base, TableMetadata metadata) {
newMetadataLocation = writeNewMetadataIfRequired(base == null, metadata);
}
@Override
CommitMode commitMode() {
return CommitMode.EXTERNAL;
}
public String metadataLocation() {
return newMetadataLocation;
}
}
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854248878
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -256,21 +258,10 @@ public void commitTransaction() {
hasLastOpCommitted, "Cannot commit transaction: last operation has not committed");
switch (type) {
- case CREATE_TABLE:
- commitCreateTransaction();
- break;
-
- case REPLACE_TABLE:
- commitReplaceTransaction(false);
- break;
-
- case CREATE_OR_REPLACE_TABLE:
- commitReplaceTransaction(true);
- break;
-
- case SIMPLE:
- commitSimpleTransaction();
- break;
+ case CREATE_TABLE -> commitCreateTransaction();
Review Comment:
While this is pretty and clean, it's not related to this PR, I would save it for a refactor PR
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854264492
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong you need to stage all table `PendingUpdates` — where would you keep them?
The client uses the Transaction API to stage multiple table changes, and `BaseTransaction` stores them in its `updates` list.
Adding prepareMetadata() to TableOps doesn’t provide any additional value and isn't even linked to a Transaction in any way.
in order to link it you should override the `commitTransaction` copy&paste plenty of things and do something like this
````
applyUpdates(underlyingOps);
underlyingOps. prepareMetadata(base, current);
````
but again `applyUpdates` & `updates` list are private
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "qlong (via GitHub)" <gi...@apache.org>.
qlong commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2849825158
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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:
```java
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2886355439
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
@dramaticlly, you made a similar change in https://github.com/apache/iceberg/pull/13631/files, so I’m not sure why a similar request is being questioned now.
https://github.com/apache/iceberg/pull/15389#discussion_r2886355439
````
We realized some difficulties to extend the BaseTransactions
// methods made protected to support extensibility in subclasses//
//
// Protected final methods available for subclasses:
// - appendUpdates(PendingUpdate): adds operations to transaction and apply essential chores
//
// Example usage in a subclass:
private static class CustomTransaction extends BaseTransaction {
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2853175067
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong, unless you expose `updates` and the rest of the internal mechanics in `BaseTransaction`, your suggestion doesn’t really help. It would be great if you could elaborate on a complete solution that doesn’t require a full rewrite of the Transaction implementation.
I could have copied `BaseTransaction`, and I don’t actually need a `prepareMetadata()` API as you’re suggesting, but that would result in significant code duplication.
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong, unless you expose `updates` and the rest of the internal mechanics in `BaseTransaction`, your suggestion doesn’t really help. It would be great if you could elaborate on a complete solution that doesn’t require a full rewrite of the Transaction implementation.
I could have copied `BaseTransaction`, and I don’t actually need a `prepareMetadata()` API as you’re suggesting, but that would result in significant code duplication.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854801633
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
> Can you confirm that the coordinator calls .commit() on each individual operation before calling commitTransaction()?
Coordinator calls `txn.commiTransaction()` for every registered table !!!! Client is using txn to stage the changes (i.e. txn.newAppend()...commit() -> `BaseTransaction`.`TransactionTableOperations`.commit(), etc).
`PendingUpdate` are staged and applied only during commiTransaction()
````
for (PendingUpdate update : updates) {
// re-commit each update in the chain to apply it and update current
update.commit();
}
underlyingOps.commit(base, current)
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2880925963
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
Kind of in the similar boat before, but do we really need this or we can use a public factory to vend out the transactions like in https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/Transactions.java? If so, we can leave the enum and constructor to be package private as is
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2838350151
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
I don't think we need any checks in rollback otherwise it's not helpfull at all. in case of multi-table commit i need to rollback regardless if staged commit succeded.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2838350151
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
It is allowed to call rollback() after commit() in JDBC.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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
--------------
As discussed, removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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
--------
As discussed, removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840106304
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
````
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) {
super(table.name(), stagingOps, TransactionType.SIMPLE, ops.current());
}
}
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2891431406
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
@deniskuzZ yeah I think here's the context https://github.com/apache/iceberg/pull/13631#discussion_r2237931079
a factory class only help with instantiation, existing package-private constructor still requires the subclass to live in the same `org.apache.iceberg` package in order to call its parent constructor.
Since this is being asked more than once, @rdblue do you think we can take this opportunity to make it protected?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2939738345
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
Since Ryan specifically asked for keeping the constructor package private, we need to talk to him and convince him that it is needed in multiple places.
Maybe add this to the next community sync and convince him with @dramaticlly that it is needed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840106304
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
````
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;
}
}
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840106304
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
````
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());
}
public HiveTransaction(Table table, HiveTableOperations ops) {
super(table.name(), stagingOps, TransactionType.SIMPLE, ops.current());
}
}
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2853175067
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong, unless you expose `updates` and the rest of the internal mechanics in `BaseTransaction`, your suggestion doesn’t really help. It would be great if you could elaborate on a complete solution that doesn’t require a full rewrite of the Transaction implementation.
The Coordinator maintains a collection of table Transactions (each holding its own set of pending updates), rather than TableOps
I could have copied `BaseTransaction`, and I don’t actually need a `prepareMetadata()` API as you’re suggesting, but that would result in significant code duplication.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854325163
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -420,8 +411,13 @@ private void commitSimpleTransaction() {
}
}
- private void cleanUpOnCommitFailure() {
- // the commit failed and no files were committed. clean up each update.
+ @Override
+ public void rollbackTransaction() {
+ cleanAll();
Review Comment:
I noted this above but this feels dangerous. We shouldn't be allowed to call this api if the transaction is already committed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854801633
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong, try calling commit on TableOps without `applyUpdates` and see what happens.
> Can you confirm that the coordinator calls .commit() on each individual operation before calling commitTransaction()?
Coordinator calls `txn.commiTransaction()` for every registered table !!!! Client is using txn to stage the changes (i.e. txn.newAppend(), etc).
`PendingUpdate` are staged and applied only during commiTransaction()
````
for (PendingUpdate update : updates) {
// re-commit each update in the chain to apply it and update current
update.commit();
}
underlyingOps.commit(base, current)
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854801633
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong, try calling commit on TableOps without `applyUpdates` and see what happens.
> Can you confirm that the coordinator calls .commit() on each individual operation before calling commitTransaction()?
Coordinator calls txn.commiTransaction() for every registered table !!!!
`PendingUpdate` are staged and applied only during commiTransaction()
````
for (PendingUpdate update : updates) {
// re-commit each update in the chain to apply it and update current
update.commit();
}
underlyingOps.commit(base, current)
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2936536159
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
updated PR, however, I’m not sure what else needs to be done to get approval for making the constructor protected.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "qlong (via GitHub)" <gi...@apache.org>.
qlong commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2853135898
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
Understand the code could work mechanically, but the core issue is that calling rollbackTransaction() (or cleanAll()) after commitTransaction() is bad transaction semantics regardless of how it is accessed (via public API or package-private). My suggestion of prepareMetadata() on HiveTableOperations avoids the problem entirely: the coordinator never calls commitTransaction() during the prepare phase, so rollbackTransaction() is always called from OPEN state, which is correct. The state guard can then be added back cleanly without any CommitMode or isStaging workaround.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854801633
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong, try calling commit on TableOps without `applyUpdates ` and see what happens.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
Multi-table commit:
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. batch table properties atomic commit using CAS (NoLock)
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840026221
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -252,25 +259,20 @@ UpdateSnapshotReferencesOperation updateSnapshotReferencesOperation() {
@Override
public void commitTransaction() {
+ Preconditions.checkState(state == State.OPEN, "Cannot commit transaction in state: %s", state);
+
Preconditions.checkState(
hasLastOpCommitted, "Cannot commit transaction: last operation has not committed");
switch (type) {
- case CREATE_TABLE:
- commitCreateTransaction();
- break;
-
- case REPLACE_TABLE:
- commitReplaceTransaction(false);
- break;
-
- case CREATE_OR_REPLACE_TABLE:
- commitReplaceTransaction(true);
- break;
+ case CREATE_TABLE -> commitCreateTransaction();
+ case REPLACE_TABLE -> commitReplaceTransaction(false);
+ case CREATE_OR_REPLACE_TABLE -> commitReplaceTransaction(true);
+ case SIMPLE -> commitSimpleTransaction();
+ }
- case SIMPLE:
- commitSimpleTransaction();
- break;
+ if (ops.commitMode() == CommitMode.AUTO) {
+ state = State.COMMITTED;
Review Comment:
I'm not sure about this one. Could you please help me understand?
Isn't it something the extended classes should handle?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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. batch table properties atomic commit using CAS (NoLock)
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the [1st commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit would remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the [1st commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit is destructive as it would delete already committed data.
However, that would require introducing an additional `CommitMode` flag (see the [1st commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2880968785
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
I want to override `BaseTransaction` to add support for staged multi-table commits.
What’s the point of making `cleanUp` protected in that case? How am I supposed to invoke it?
We could keep the enum package-private, but in that case, we’d need a default constructor that sets transactionType to `SIMPLE`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2838350151
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
It is allowed to call rollback() after commit() in JDBC.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2838350151
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
Done, added `State` enum.
Also had to introduce `CommitMode` enum for external commit coordination (multi-table txns)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840106304
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
````
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) {
super(table.name(), stagingOps, TransactionType.SIMPLE, ops.current());
}
}
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
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](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840106304
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
````
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());
}
}
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2837597477
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
I'd make it a state enum with an atomic reference, then a checkTransactionState(TransactionState) before nearly all calls.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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. batch table properties atomic commit using CAS (NoLock)
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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
}
5. batch table properties atomic commit using CAS (NoLock)
6. release locks if any
7. rollback ALL if table has been modified concurrently or exception
removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854801633
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong, try calling commit on TableOps without `applyUpdates` and see what happens.
> Can you confirm that the coordinator calls .commit() on each individual operation before calling commitTransaction()?
Coordinator calls txn.commiTransaction() !!!!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "qlong (via GitHub)" <gi...@apache.org>.
qlong commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854235075
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
My suggestion is to keep transaction semantics correct (eg, no rollback after commit). The only change needed for this PR is to add state guard in BaseTransaction, then it should support your multi-table commit.
For the muti table commit, two small changes to HiveTableOperations:
1. Add prepareMetadata() — one new method using the already-protected writeNewMetadataIfRequired()
2. Check stagedMetadataLocation in HiveTableOperations.doCommit() to skip the duplicate S3 write — two lines
The coordinator change is also small: call hiveOps.prepareMetadata() before commitTransaction() instead of using StagingTableOperations for staging. The coordinator already holds the HiveTableOperations reference.
StagIngTableOpererations is no longer needed in this commit flow.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854801633
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong, try calling commit on TableOps without `applyUpdates` and see what happens.
> Can you confirm that the coordinator calls .commit() on each individual operation before calling commitTransaction()?
Coordinator calls txn.commiTransaction() for every registered table !!!!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854264492
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong you need to stage all table `PendingUpdates` — where would you keep them?
The client uses the Transaction API to stage multiple table changes, and `BaseTransaction` stores them in its `updates` list.
Adding prepareMetadata() to TableOps doesn’t provide any real value and in your design it's not even linked to a `Transaction` in any way or i might be missing something.
in order to link - you should override the `commitTransaction` API, copy&paste plenty of things and do something like this
````
applyUpdates(underlyingOps);
underlyingOps. prepareMetadata(base, current);
````
but again `applyUpdates` & `updates` list are private
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854801633
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
> Can you confirm that the coordinator calls .commit() on each individual operation before calling commitTransaction()?
Coordinator calls `txn.commiTransaction()` for every registered table !!!! Client is using txn to stage the changes (i.e. txn.newAppend()...commit(), etc).
`PendingUpdate` are staged and applied only during commiTransaction()
````
for (PendingUpdate update : updates) {
// re-commit each update in the chain to apply it and update current
update.commit();
}
underlyingOps.commit(base, current)
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840106304
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
````
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 StagingTransaction extends BaseTransaction {
...
public StagingTransaction(Table table, HiveTableOperations ops) {
this(table, ops, ops.toStagingOps());
}
private StagingTransaction(Table table, HiveTableOperations ops) {
super(table.name(), stagingOps, TransactionType.SIMPLE, ops.current());
}
}
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2838350151
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
I don't think we need any checks in rollback, otherwise it's not helpfull at all. in case of multi-table commit i need to rollback if table metadata location has changed.
FYI, it is allowed to call rollback() after commit() in JDBC.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854264492
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong you need to stage all table `PendingUpdates` — where would you keep them?
The client uses the Transaction API to stage multiple table changes, and `BaseTransaction` stores them in its `updates` list.
Adding prepareMetadata() to TableOps doesn’t provide any additional value and isn't even linked to a Transaction in any way.
in order to link it you should do something like this
````
applyUpdates(underlyingOps);
underlyingOps. prepareMetadata(base, current);
````
but again `applyUpdates` & `updates` list are private
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854264492
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong you need to stage all table `PendingUpdates` — where would you keep them?
The client uses the Transaction API to stage multiple table changes, and `BaseTransaction` stores them in its `updates` list.
Adding prepareMetadata() to TableOps doesn’t provide any additional value and isn't even linked to a Transaction in any way.
in order to link it you should override the `commitTransaction` API, copy&paste plenty of things and do something like this
````
applyUpdates(underlyingOps);
underlyingOps. prepareMetadata(base, current);
````
but again `applyUpdates` & `updates` list are private
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854362608
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -420,8 +411,13 @@ private void commitSimpleTransaction() {
}
}
- private void cleanUpOnCommitFailure() {
- // the commit failed and no files were committed. clean up each update.
+ @Override
+ public void rollbackTransaction() {
+ cleanAll();
Review Comment:
@RussellSpitzer, please check discussion here https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "qlong (via GitHub)" <gi...@apache.org>.
qlong commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854235075
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
My suggestion is to keep transaction semantics correct (eg, no rollback after commit). The only change needed for this PR is to add state guard in BaseTransaction, then it should support your multi-table commit.
For the muti table commit, two small changes to HiveTableOperations:
1. Add prepareMetadata() — one new method using the already-protected writeNewMetadataIfRequired()
2. Check stagedMetadataLocation in HiveTableOperations.doCommit() to skip the duplicate S3 write — two lines
The coordinator change is also small: call hiveOps.prepareMetadata() before commitTransaction() instead of using StagingTableOperations for staging. The coordinator already holds the HiveTableOperations reference.
StagIngTableOpererations is also not needed in this commit flow.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854264492
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong you need to stage all table PendingUpdates, where would you keep them ???? BaseTransaction stages them in updates list. prepareMetadata() on TableOps doesn't add any value
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2880968785
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
Which factory are you referring to? I want to override `BaseTransaction` to add support for staged multi-table commits.
What’s the point of making `cleanUp` protected in that case? How am I supposed to invoke it?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "qlong (via GitHub)" <gi...@apache.org>.
qlong commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854542298
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
Can you confirm that the coordinator calls .commit() on each individual operation before calling commitTransaction()?
If yes, the prepareMetadata approach should work.
> you need to stage all table PendingUpdates — where would you keep them?
They are already applied. Each individual PendingUpdate.commit() call writes manifests to S3 and updates BaseTransaction.current in memory. txn.currentMetadata() is the fully computed result — prepareMetadata() just serializes it to JSON and writes it to S3, exactly as StagingTableOperations.doCommit() does today.
> Adding prepareMetadata() to TableOps doesn't provide any additional value and isn't even linked to a Transaction in any way.
prepareMetadata() **is** linked to the transaction through txn.currentMetadata().prepareMetadata() receives the final computed data from txn, it does not need access to updates or applyUpdates() at all.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "qlong (via GitHub)" <gi...@apache.org>.
qlong commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2849825158
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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:
```java
public String prepareMetadata(TableMetadata metadata) {
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854310646
##########
api/src/main/java/org/apache/iceberg/Transaction.java:
##########
@@ -183,4 +183,15 @@ default ManageSnapshots manageSnapshots() {
* @throws CommitFailedException If the updates cannot be committed due to conflicts.
*/
void commitTransaction();
+
+ /**
+ * Rollback the transaction by cleaning up uncommitted changes.
Review Comment:
I'm not sure about the name Rollback here, since we already use "rollback" to talk about returning whole table state.
https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/ManageSnapshots.java#L62-L72
This on the other hand is not actually "rolling back" a change that happened it is instead just erasing any files associated with the transaction. We also probably need a protection that this can't be called if "commitTransaction" has been called?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2880968785
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
Which factory are you referring to? I want to override `BaseTransaction` to add support for staged multi-table commits.
What’s the point of making `cleanUp` protected in that case? How am I supposed to invoke it?
We could keep the enum package-private, but in that case, we’d need a default constructor that sets transactionType to SIMPLE.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2880968785
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
Which factory are you referring to? I want to override `BaseTransaction` to add support for staged multi-table commits.
What’s the point of making `cleanUp` protected in that case? How am I supposed to invoke it?
We could keep the enum package-private, but in that case, we’d need a default constructor that sets transactionType to `SIMPLE`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2835635262
##########
api/src/main/java/org/apache/iceberg/Transaction.java:
##########
@@ -183,4 +183,12 @@ default ManageSnapshots manageSnapshots() {
* @throws CommitFailedException If the updates cannot be committed due to conflicts.
*/
void commitTransaction();
+
+ /**
+ * Abort the transaction by cleaning up uncommitted changes.
+ *
+ * <p>This cleans up any files produced by pending updates and deletes any uncommitted files
+ * tracked by the transaction.
+ */
+ void abortTransaction();
Review Comment:
I think we might need to add a default method for this new method to avoid break existing class that implemented the Transaction interface.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
Multi-table commit:
table loop {
1. stage every change: `BaseTransaction.commit` -> `StagingTableOperations.doCommit`, writes metadata file only, skipping the locking, HMS metadata file update
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) -> new multi-table batch API in HMS
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
--------
As discussed, removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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 - ATM, we rely on reflection to invoke `BaseTransaction#cleanUpOnCommitFailure`
--------
As discussed, removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
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` flag ( (see the [1st commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2838350151
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
I don't think we need any checks in rollback, otherwise it's not helpfull at all. in case of multi-table commit i need to rollback if table metadata location has changed.
FYI, it is allowed to call rollback() after commit() in JDBC
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #15389:
URL: https://github.com/apache/iceberg/pull/15389
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2880968785
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
Which factory are you referring to? I’d like to override BaseTransaction to add support for multi-table commits.
What’s the point of making cleanUp protected in that case?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2880968785
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
Which factory are you referring to? I’d like to override BaseTransaction to add support for multi-table commits.
What’s the point of making cleanUp protected in that case? How am I supposed to invoke it?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [API] Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#issuecomment-3937500710
should we introduce isAborted flag?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2849917974
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
Multi-table commit:
table loop {
1. stage every change: `BaseTransaction.commit` -> `StagingTableOperations.doCommit`, writes metadata file only, skipping the locking, HMS metadata file update
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) -> new multi-table batch API in HMS
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
--------
As discussed, removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit removes already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit is destructive as it would delete already committed data; however, that would require introducing an additional `CommitMode` flag (see the [1st commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840164523
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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 - ATM, we rely on reflection to invoke `BaseTransaction#cleanUpOnCommitFailure` and that is the purpose of this PR
--------
As discussed, removed the guards in [2nd commit](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the [1st commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2838350151
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
I don't think we need any checks in rollback, otherwise it's not helpfull at all. in case of multi-table commit i need to rollback regardless if staged commit succeded.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2838350151
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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. batch table properties atomic commit using CAS (NoLock)
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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. batch table properties atomic commit using CAS (NoLock)
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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. batch table properties atomic commit using CAS (NoLock)
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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 - ATM, we rely on reflection to invoke `BaseTransaction#cleanUpOnCommitFailure`
--------
As discussed, removed the guards in [2nd commit](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the [first commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854801633
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
> Can you confirm that the coordinator calls .commit() on each individual operation before calling commitTransaction()?
Coordinator calls `txn.commiTransaction()` for every registered table !!!! Client is using txn to stage the changes (i.e. txn.newAppend(), etc).
`PendingUpdate` are staged and applied only during commiTransaction()
````
for (PendingUpdate update : updates) {
// re-commit each update in the chain to apply it and update current
update.commit();
}
underlyingOps.commit(base, current)
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2880968785
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
I want to override `BaseTransaction` to add support for staged multi-table commits.
What’s the point of making `cleanUp` protected in that case? How am I supposed to invoke it?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#issuecomment-4066956637
Once the tests are green, I will merge this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
Multi-table commit:
1. stage changes for all tables: `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. batch table properties atomic commit using CAS (NoLock)
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840106304
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
````
public class StagingTableOperations extends HiveTableOperations {
private String newMetadataLocation;
public StagingTableOperations(
Configuration conf,
ClientPool<IMetaStoreClient, TException> metaClients,
FileIO fileIO,
String catalogName,
String database,
String table) {
super(conf, metaClients, fileIO, 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;
}
}
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2838350151
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
I don't think we need any checks in rollback, otherwise it's not helpfull at all. in case of multi-table commit i need to rollback if table metadata location has changed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit would remove already committed manifest files; however, that would require introducing an additional `CommitMode` flag (see the [1st commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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 - ATM, we rely on reflection to invoke `BaseTransaction#cleanUpOnCommitFailure`
--------
As discussed, removed the guards in [2nd commit](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the [1st commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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 - ATM, we rely on reflection to invoke `BaseTransaction#cleanUpOnCommitFailure`
--------
As discussed, removed the guards in [2nd commit](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854264492
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong you need to stage all table `PendingUpdates` — where would you keep them?
The client uses the Transaction API to stage multiple table changes, and `BaseTransaction` stores them in its updates list.
Adding prepareMetadata() to TableOps doesn’t provide any additional value and isn't even linked to a Transaction in any way.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854264492
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong you need to stage all table PendingUpdates — where would you keep them?
The client is already using the Transaction API to stage multiple table changes, and BaseTransaction stores them in its updates list. Adding prepareMetadata() to TableOps doesn’t provide any additional value.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
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` flag (see the [1st commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2849917974
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@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.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2853175067
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong, unless you expose `updates` and the rest of the internal mechanics in `BaseTransaction`, your suggestion doesn’t really help. It would be great if you could elaborate on a complete solution that doesn’t require a full rewrite of the Transaction implementation.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854264492
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong you need to stage all table `PendingUpdates` — where would you keep them?
The client uses the Transaction API to stage multiple table changes, and `BaseTransaction` stores them in its `updates` list.
Adding prepareMetadata() to TableOps doesn’t provide any additional value and isn't even linked to a Transaction in any way.
in order to link it you should do something like this
````
applyUpdates(underlyingOps);
underlyingOps. prepareMetadata(base, current);
````
but again `updates` list is private
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854264492
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong you need to stage all table `PendingUpdates` — where would you keep them?
The client uses the Transaction API to stage multiple table changes, and `BaseTransaction` stores them in its `updates` list.
Adding prepareMetadata() to TableOps doesn’t provide any additional value and isn't even linked to a Transaction in any way.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854264492
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong you need to stage all table `PendingUpdates` — where would you keep them?
The client uses the Transaction API to stage multiple table changes, and BaseTransaction stores them in its updates list. Adding prepareMetadata() to TableOps doesn’t provide any additional value and isn't even linked to a Transaction in any way.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2835955281
##########
api/src/main/java/org/apache/iceberg/Transaction.java:
##########
@@ -183,4 +183,12 @@ default ManageSnapshots manageSnapshots() {
* @throws CommitFailedException If the updates cannot be committed due to conflicts.
*/
void commitTransaction();
+
+ /**
+ * Abort the transaction by cleaning up uncommitted changes.
+ *
+ * <p>This cleans up any files produced by pending updates and deletes any uncommitted files
+ * tracked by the transaction.
+ */
+ void abortTransaction();
Review Comment:
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
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2837343793
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
Could it be isAbortedOrCommitted? Or is Finalized?
We lso need to prevent an abort when the Transaction was committed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2838350151
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
I don't think we need any checks in rollback, otherwise it's not helpfull at all. in case of multi-table commit i need to rollback if table metadata location has changed.
FYI, it is allowed to call rollback() after commit() in JDBC. same applies to commit() after the rollback()
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#issuecomment-3941498494
> given the subclassing
subclassing is not allowed, constructor is package-private!!!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#issuecomment-3941747417
> 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
added tests.
@steveloughran, FYI subclassing is restricted by package-private constructor
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce an abortTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2838350151
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
done. had to introduce CommitMode enum for external commit coordination (multi-table txns)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
Multi-table commit:
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. batch table properties atomic commit using CAS (NoLock)
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional CommitMode flag (see the first commit).
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
Multi-table commit:
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. batch table properties atomic commit using CAS (NoLock)
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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](https://github.com/apache/iceberg/pull/15389/changes/6719771c5894e482a552679510b095de57e2df12).
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` flag (see the [1st commit](https://github.com/apache/iceberg/pull/15389/changes/e4ba7b3b4afa2647fa0f2a6a6d2d1124c799674f#diff-fb713bd8803fd14e254ff3cb7ceb44183e2ceef244d9b011f8e08df23aa662bfR274-R275)).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854801633
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
@qlong, try calling commit on TableOps without `applyUpdates` and see what happens.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2854476016
##########
api/src/main/java/org/apache/iceberg/Transaction.java:
##########
@@ -183,4 +183,15 @@ default ManageSnapshots manageSnapshots() {
* @throws CommitFailedException If the updates cannot be committed due to conflicts.
*/
void commitTransaction();
+
+ /**
+ * Rollback the transaction by cleaning up uncommitted changes.
Review Comment:
cleanUpOnRollback?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2911043649
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
Here is my suggestion:
- Don't change any previous decisions for now
- Keep the constructors package private
- Rename cleanUpOnCommitFailure to cleanUp and make it protected - to show that it is used elsewhere
Merge this and change the BaseTransaction visibility later if we decide so.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840106304
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
````
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 StagingTransaction 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());
}
}
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
Multi-table commit:
table loop {
1. stage every change: `BaseTransaction.commit` -> `StagingTableOperations.doCommit`, writes metadata file only, skipping the locking, HMS metadata file update
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
--------
As discussed, removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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. batch table properties atomic commit using CAS (NoLock)
5. release locks if any
6. rollback ALL if table has been modified concurrently or exception
removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] API: Introduce rollbackTransaction() API [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2840290133
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -78,6 +78,7 @@ enum TransactionType {
private TableMetadata base;
private TableMetadata current;
private boolean hasLastOpCommitted;
+ private boolean isAborted = false;
Review Comment:
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
removed the guards in 2nd commit.
I think having guards in place makes sense to prevent ending up in an inconsistent state. Calling rollback after commit could potentially remove already committed data; however, that would require introducing an additional `CommitMode` flag (see the first commit).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2936536159
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
updated PR
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#issuecomment-4067087605
Merged to main.
Thanks @deniskuzZ for the PR and implementing all of the different versions, and for all of the reviewers for their time and thoughts!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Rename cleanUpOnCommitFailure to cleanUp and make it protected [iceberg]
Posted by "deniskuzZ (via GitHub)" <gi...@apache.org>.
deniskuzZ commented on code in PR #15389:
URL: https://github.com/apache/iceberg/pull/15389#discussion_r2886355439
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -59,7 +59,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG = LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
@dramaticlly, you made a similar change in https://github.com/apache/iceberg/pull/13631/files, so I’m not sure why a similar request is being questioned now.
````
We realized some difficulties to extend the BaseTransactions
// methods made protected to support extensibility in subclasses//
//
// Protected final methods available for subclasses:
// - appendUpdates(PendingUpdate): adds operations to transaction and apply essential chores
//
// Example usage in a subclass:
private static class CustomTransaction extends BaseTransaction {
````
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org