szehon-ho commented on code in PR #5622:
URL: https://github.com/apache/iceberg/pull/5622#discussion_r994921586
##########
spark/v3.0/spark/src/main/java/org/apache/iceberg/spark/actions/BaseMigrateTableSparkAction.java:
##########
@@ -58,6 +58,7 @@
private final StagingTableCatalog destCatalog;
private final Identifier destTableIdent;
private final Identifier backupIdent;
+ private boolean removeBackup = false;
Review Comment:
Nit: we try to put a space between finals and non-finals as well
##########
api/src/main/java/org/apache/iceberg/actions/MigrateTable.java:
##########
@@ -41,6 +41,14 @@ public interface MigrateTable extends Action<MigrateTable,
MigrateTable.Result>
*/
MigrateTable tableProperty(String name, String value);
+ /**
+ * Sets the dropBackup property to drop backup table after a successful
table migration.
+ *
+ * @param dropBackup If set to true, removes the backup table
Review Comment:
"if true, removes the backup table"
##########
spark/v3.0/spark/src/main/java/org/apache/iceberg/spark/actions/BaseMigrateTableSparkAction.java:
##########
@@ -221,4 +230,12 @@ private void restoreSourceTable() {
e);
}
}
+
+ private void dropBackupTable() {
+ try {
+ destCatalog().dropTable(backupIdent);
+ } catch (Exception e) {
+ LOG.error("Cannot drop the backup table {} after migrate is completed.",
backupIdent, e);
Review Comment:
Can you change the rest of the Spark files? "after" should be used before
a noun, and "migrate" is a verb.
##########
api/src/main/java/org/apache/iceberg/actions/MigrateTable.java:
##########
@@ -41,6 +41,14 @@ public interface MigrateTable extends Action<MigrateTable,
MigrateTable.Result>
*/
MigrateTable tableProperty(String name, String value);
+ /**
+ * Sets the dropBackup property to drop backup table after a successful
table migration.
Review Comment:
Sentence is a bit redudant, you can see DeleteOrphanFiles or ExpireSnapshots
for more direct examples.
The previous methods says 'sets a table property' because that's what it
does, but here there's no point to say it sets a property. How about: 'Drops
the backup of the original table after a successful migration'.
--
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]
