dramaticlly commented on code in PR #13631:
URL: https://github.com/apache/iceberg/pull/13631#discussion_r2238062233
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -121,180 +125,143 @@ public TableOperations underlyingOps() {
return ops;
}
- private void checkLastOperationCommitted(String operation) {
+ protected PendingUpdate appendUpdates(PendingUpdate update) {
+ if (!(update instanceof ManageSnapshots)) {
+ checkLastOperationCommitted(update.getClass().getSimpleName());
Review Comment:
Yes, I think this is only used to handle the `manageSnapshots` case.
I cam move the logic inside `checkLastOperationCommitted`, where still check
last operation is committed but delay setting the flag for manageSnapshots
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -56,7 +56,7 @@
public class BaseTransaction implements Transaction {
private static final Logger LOG =
LoggerFactory.getLogger(BaseTransaction.class);
- enum TransactionType {
+ protected enum TransactionType {
Review Comment:
Right, I think this enum is only used to initialize the transaction, I think
we can create a class similar to Transactions in package `org.apache.iceberg`
to vend derived class
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -121,180 +121,143 @@ public TableOperations underlyingOps() {
return ops;
}
- private void checkLastOperationCommitted(String operation) {
+ protected <T extends PendingUpdate> T appendUpdates(T update) {
+ if (!(update instanceof ManageSnapshots)) {
+ checkLastOperationCommitted(update.getClass().getSimpleName());
+ }
+ if (update instanceof SnapshotUpdate) {
Review Comment:
I can follow up with a different PR to see the scope of the change. I did a
quick try locally but might need a bit more digging to maintain the original
return type to allow method chaining.
##########
core/src/main/java/org/apache/iceberg/BaseTransaction.java:
##########
@@ -121,180 +121,143 @@ public TableOperations underlyingOps() {
return ops;
}
- private void checkLastOperationCommitted(String operation) {
+ protected <T extends PendingUpdate> T appendUpdates(T update) {
+ if (!(update instanceof ManageSnapshots)) {
+ checkLastOperationCommitted(update.getClass().getSimpleName());
+ }
+ if (update instanceof SnapshotUpdate) {
+ ((SnapshotUpdate) update).deleteWith(enqueueDelete);
+ }
+ if (update instanceof SnapshotProducer) {
+ ((SnapshotProducer) update).reportWith(reporter);
+ }
+ this.updates.add(update);
+ return update;
+ }
+
+ void checkLastOperationCommitted(String operation) {
Review Comment:
moved to protected instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
