Spark: Refactor snapshot and migrate actions #2437

aokolnychyi · 2021-04-07T22:33:20Z

This PR moves our snapshot and migrate actions to use the new API.

spark3/src/main/java/org/apache/iceberg/spark/actions/BaseMigrateTableSparkAction.java

spark3/src/main/java/org/apache/iceberg/spark/actions/BaseSnapshotTableSparkAction.java

aokolnychyi · 2021-04-07T22:35:22Z

spark3/src/main/java/org/apache/iceberg/spark/actions/BaseSnapshotTableSparkAction.java

+  }
+
+  @Override
+  public SnapshotTable tableLocation(String location) {


This check will have to be refined separately.

spark3/src/main/java/org/apache/iceberg/spark/actions/BaseTableMigrationSparkAction.java

aokolnychyi · 2021-04-07T22:35:55Z

spark3/src/main/java/org/apache/iceberg/spark/actions/BaseTableMigrationSparkAction.java

-  public CreateAction withProperties(Map<String, String> properties) {
-    this.additionalProperties.putAll(properties);
-    return this;
+  protected void setDestCatalogAndIdent(CatalogPlugin catalog, Identifier ident) {


Called from as.

We let you use "as" with Migration? Maybe I'm forgetting how this worked.

I think this was needed before since Snapshot and Migrate descended from CreateTable Actions, now I don't think you need dest catalog at all

This is still the parent class where we need to reference the dest catalog. However, I think your idea makes sense. We can probably remove the dest fields from the parent class as they are initialized differently now. Then we don't have to do that weird if statement in the constructor.

I've pushed an update. Let me know if you prefer the old approach.

spark3/src/main/java/org/apache/iceberg/spark/actions/BaseTableMigrationSparkAction.java

aokolnychyi · 2021-04-07T22:36:39Z

cc @RussellSpitzer

RussellSpitzer · 2021-04-07T23:25:24Z

core/src/main/java/org/apache/iceberg/actions/BaseSnapshotTableActionResult.java

+
+  @Override
+  public long importedDataFilesCount() {
+    return importedDataFilesCount;


Not for this pr, but now that I think about it we should probably also let the user know how many metadata files were created as well

Good idea! Now that we have the result interface, we can evolve it. Could you create an issue for this, @RussellSpitzer?

@flyrain, would you be interested to pick it up?

aokolnychyi · 2021-04-08T17:15:48Z

I see more and more Tez-related failures on recent PRs.

org.apache.iceberg.mr.hive.TestHiveIcebergStorageHandlerWithEngine > testCBOWithSelectedColumnsOverlapJoin[fileFormat=AVRO, engine=tez, catalog=HIVE_CATALOG] FAILED
    java.lang.IllegalArgumentException: Failed to execute Hive query 'SELECT c.first_name, o.order_id FROM default.orders o JOIN default.customers c ON o.customer_id = c.customer_id ORDER BY o.order_id DESC': Error while processing statement: FAILED: Execution Error, return code 1 from org.apache.hadoop.hive.ql.exec.tez.TezTask
        at org.apache.iceberg.mr.hive.TestHiveShell.executeStatement(TestHiveShell.java:151)
        at org.apache.iceberg.mr.hive.TestHiveIcebergStorageHandlerWithEngine.testCBOWithSelectedColumnsOverlapJoin(TestHiveIcebergStorageHandlerWithEngine.java:217)

cc @pvary @marton-bod

pvary · 2021-04-08T17:51:43Z

I see more and more Tez-related failures on recent PRs.

Could you please provide a link for any of the failed test runs? I would like to get the logs for the failed tests.

Thanks,
Peter

aokolnychyi · 2021-04-08T18:01:54Z

@pvary, here is one run

https://github.com/apache/iceberg/pull/2362/checks?check_run_id=2290454777

pvary · 2021-04-09T06:23:13Z

@pvary, here is one run

https://github.com/apache/iceberg/pull/2362/checks?check_run_id=2290454777

I see this at the end of the logs:

2021-04-08T01:04:25.1453911Z ##[error]The operation was canceled.

Was it done manually, or it was cancelled automatically because of the failures?

I am asking because some time ago (in #1789) I have introduced a feature to collect the test logs in exactly the same situations that happen here (flaky test failures on CI). This created a test logs artifact for the failed runs. This log is not available for this run.

If the run was cancelled automatically then I have to check what changed around the build process. OTOH if the run was cancelled manually then I need to find a non-cancelled run with the same failures.

Thanks,
Peter

aokolnychyi · 2021-04-09T07:10:56Z

Hmm, I think you analysis is right, @pvary. It looks like we are hitting timeouts, though. The latest CI job on this PR, for example, failed with the timeout of 360 minutes.

aokolnychyi · 2021-04-09T07:11:14Z

I'll try to see what causes the timeout tomorrow.

aokolnychyi · 2021-04-09T07:13:12Z

Looks like the job I pointed to also failed with a timeout.

pvary · 2021-04-09T07:25:02Z

Looks like the job I pointed to also failed with a timeout.

Maybe this is related to resource problems when trying to create the Tez session?

When we are creating a new TezAM we ask a new container from YARN. Maybe the issue is that we do not get the new Yarn container (because of the missing resources) and we waiting until the timeout is reached. It might be a good idea to add @Timeout for the tests

aokolnychyi · 2021-04-09T20:22:35Z

Rebased this one. @RussellSpitzer, could you take a look?

RussellSpitzer · 2021-04-12T18:18:11Z

spark3/src/main/java/org/apache/iceberg/spark/actions/BaseTableMigrationSparkAction.java


-  Spark3CreateAction(SparkSession spark, CatalogPlugin sourceCatalog, Identifier sourceTableIdent,
-                     CatalogPlugin destCatalog, Identifier destTableIdent) {
+  BaseTableMigrationSparkAction(SparkSession spark, CatalogPlugin sourceCatalog, Identifier sourceTableIdent) {


I liked having this be a non "migrate - snapshot" name here because I didn't want any confusion if there was an error during "snapshots" that made it look like it was doing a migrate because of the trace.

RussellSpitzer

I think this makes sense to me, My only real issue is I think the namings are a little confusing,

We have
BaseTableMigrationSparkAction
| - BaseMigrateTableSparkAction
| - BaseSparkTableSnapshot Action

I think the base class here should be "Create" or something more visually distinct because currently we have TableMigration and MigrateTable, and as I noted I think we should keep our Snapshot traces clear of any references to migrating.

That said I wouldn't hold this back based on naming, so feel free to ignore.

I still think BaseTableCreationSparkAction is fine :)

aokolnychyi · 2021-04-12T20:51:29Z

Agree, @RussellSpitzer. Let me fix it.

aokolnychyi · 2021-04-13T04:03:00Z

Thanks for reviewing, @RussellSpitzer!

github-actions bot added core spark labels Apr 7, 2021

aokolnychyi commented Apr 7, 2021

View reviewed changes

spark3/src/main/java/org/apache/iceberg/spark/actions/BaseMigrateTableSparkAction.java Outdated Show resolved Hide resolved

aokolnychyi commented Apr 7, 2021

View reviewed changes

spark3/src/main/java/org/apache/iceberg/spark/actions/BaseSnapshotTableSparkAction.java Outdated Show resolved Hide resolved

aokolnychyi commented Apr 7, 2021

View reviewed changes

spark3/src/main/java/org/apache/iceberg/spark/actions/BaseTableMigrationSparkAction.java Outdated Show resolved Hide resolved

aokolnychyi commented Apr 7, 2021

View reviewed changes

spark3/src/main/java/org/apache/iceberg/spark/actions/BaseTableMigrationSparkAction.java Show resolved Hide resolved

RussellSpitzer reviewed Apr 7, 2021

View reviewed changes

pvary mentioned this pull request Apr 9, 2021

Hive: Add timeout for TestHiveIcebergStorageHandlerWithEngine tests #2448

Merged

aokolnychyi force-pushed the v2-snapshot-table-action branch from f5ec56f to 54ec4bd Compare April 9, 2021 20:11

Spark: Refactor snapshot and migrate actions

90aacfb

aokolnychyi force-pushed the v2-snapshot-table-action branch from 54ec4bd to 90aacfb Compare April 9, 2021 20:14

Remove dest catalog from parent

bbb2106

RussellSpitzer reviewed Apr 12, 2021

View reviewed changes

RussellSpitzer approved these changes Apr 12, 2021

View reviewed changes

Rename base class to BaseTableCreationSparkAction

1ed5c77

aokolnychyi merged commit 6eaabb5 into apache:master Apr 13, 2021

stevenzwu pushed a commit to stevenzwu/iceberg that referenced this pull request Jul 28, 2021

Spark: Refactor snapshot and migrate actions (apache#2437)

2d4a789

Spark: Refactor snapshot and migrate actions #2437

Spark: Refactor snapshot and migrate actions #2437

Uh oh!

Conversation

aokolnychyi commented Apr 7, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aokolnychyi commented Apr 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi commented Apr 8, 2021

Uh oh!

pvary commented Apr 8, 2021

Uh oh!

aokolnychyi commented Apr 8, 2021

Uh oh!

pvary commented Apr 9, 2021

Uh oh!

aokolnychyi commented Apr 9, 2021

Uh oh!

aokolnychyi commented Apr 9, 2021

Uh oh!

aokolnychyi commented Apr 9, 2021

Uh oh!

pvary commented Apr 9, 2021

Uh oh!

aokolnychyi commented Apr 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi commented Apr 12, 2021

Uh oh!

aokolnychyi commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RussellSpitzer left a comment •

edited

Loading