Spark : Support parallelism in RemoveOrphanFiles #3872

dramaticlly · 2022-01-10T20:11:55Z

Notes

Allow parallelism in removing orphan files using Spark procedures and Spark Actions, like how it was implemented in expire snapshots, This change close #3582

Since I need to change the DeleteOrphanFiles interface in api, ends up also updating the code for spark2.4/3.0/3.1/3.2 BaseDeleteOrphanFilesSparkAction as they are implement the same interface, also update the spark procedures to allow calling from spark SQL.

Tests

I update the tests for TestRemoveOrphanFilesAction and TestRemoveOrphanFilesProcedure in respective spark version. I did not touch any tests in TestRemoveOrphanFilesAction3 as it mostly cover different catalogs

$ ./gradlew -DsparkVersions=3.2 \
-DhiveVersions= -DflinkVersions= \
:iceberg-spark:iceberg-spark-3.2_2.12:check \
:iceberg-spark:iceberg-spark-extensions-3.2_2.12:check \
:iceberg-spark:iceberg-spark-runtime-3.2_2.12:check \
-Pquick=true -x javadoc

BUILD SUCCESSFUL in 29s
54 actionable tasks: 41 executed, 13 up-to-date

dramaticlly · 2022-01-10T20:28:58Z

@szehon-ho and @RussellSpitzer , can you help kicked of the CI workflow for me as first time contributor? Feel free to take a look and I can do more local testing in the meantime. Thank you!

szehon-ho · 2022-01-10T21:23:32Z

Thanks, will take a look! I'll have to let others as I dont have permission

RussellSpitzer · 2022-01-11T02:02:05Z

Approved to run

dramaticlly · 2022-01-11T20:57:12Z

Updated Spark 2.4/3.0/3.1 Actions due to interface change in api, somehow it was not reflected in IDE initially as those spark version was not added as src.

Anyway I did run the check and test for each spark version and they are all passing, and also rebased with latest upstream. Hopefully CI would be happy now

dramaticlly · 2022-01-11T23:26:25Z

Sorry, I think I did some rebase from upstream and fi, ends up re-require the approval in order to run the CI. But now this PR it's ready for review and I was wondering if @RussellSpitzer can help take a look?

Add @szehon-ho as well for issue #3582

RussellSpitzer · 2022-01-12T03:08:52Z

Sorry I've only been able to approve the ci, busy Monday and Tuesday!

dramaticlly · 2022-01-12T03:51:51Z

Sorry I've only been able to approve the ci, busy Monday and Tuesday!

No worries, please take your time and I am happy to see all checks are passing

...4/spark/src/main/java/org/apache/iceberg/spark/actions/BaseDeleteOrphanFilesSparkAction.java

RussellSpitzer · 2022-01-13T16:16:38Z

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestRemoveOrphanFilesProcedure.java

+
+  @Test
+  public void testConcurrentRemoveOrphanFilesWithInvalidInput() {
+    sql("CREATE TABLE %s (id bigint NOT NULL, data string) USING iceberg", tableName);


We usually add these tests to testInvalid***Cases()

I see we did it this way in ExpireSnapshots too... I guess it's fine to do it that way here as well

RussellSpitzer

This looks good to me, Small request though

Can we just put the changes for Spark 3.2 in this pr then the backports in another pr?

dramaticlly · 2022-01-13T16:28:09Z

This looks good to me, Small request though

Can we just put the changes for Spark 3.2 in this pr then the backports in another pr?

Thanks Russel, that's my original intention as well. But since I am change the interface in API, change the spark 3.2 alone but not other Spark versions will break the CI workflow. Or do you want me to separate these change out in 2 separate commits but in same PR?

Maybe I understand you wrong, If you can elaborate a bit more in detail, I am happy to do it!

RussellSpitzer · 2022-01-13T16:29:39Z

Ah I totally forgot, I think it's fine then. It just makes it harder for me to check all the changes. I'll merge after we fix up the few spacing nits.

dramaticlly · 2022-01-13T16:44:08Z

@RussellSpitzer, I added newline as suggested by you in baseAction for spark 2.4/3.0/3.1/3.2 and confirmed that they are all the same before and after my change, I hope that helps!

Also looks like CI still need your approval to run for the spacing change

➜  iceberg git:(master) diff spark/v2.4/spark/src/main/java/org/apache/iceberg/spark/actions/BaseDeleteOrphanFilesSparkAction.java spark/v3.0/spark/src/main/java/org/apache/iceberg/spark/actions/BaseDeleteOrphanFilesSparkAction.java
➜  iceberg git:(master) diff spark/v2.4/spark/src/main/java/org/apache/iceberg/spark/actions/BaseDeleteOrphanFilesSparkAction.java spark/v3.1/spark/src/main/java/org/apache/iceberg/spark/actions/BaseDeleteOrphanFilesSparkAction.java
➜  iceberg git:(master) diff spark/v2.4/spark/src/main/java/org/apache/iceberg/spark/actions/BaseDeleteOrphanFilesSparkAction.java spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/actions/BaseDeleteOrphanFilesSparkAction.java

RussellSpitzer · 2022-01-13T16:47:03Z

Yep! Clicked it off, you will have full auth to run any CI after we merge this so you won't need me anymore :D

szehon-ho

Looks great to me

szehon-ho · 2022-01-14T18:19:51Z

...1/spark/src/main/java/org/apache/iceberg/spark/actions/BaseDeleteOrphanFilesSparkAction.java

@@ -86,6 +87,8 @@
    }
  }, DataTypes.StringType);

+  private static final ExecutorService DEFAULT_DELETE_EXECUTOR_SERVICE = null;


Not sure I see a huge point to this variable (feel like a comment could have sufficed), but I guess it's there in the ExpireSnapshot version, so no matter :)

I guess the explicit assumption is that Tasks if executing without a service, executes the task on a single thread.

RussellSpitzer · 2022-01-14T18:21:56Z

Thanks @dramaticlly for the PR and @szehon-ho for the review! Merged!

github-actions bot added API spark labels Jan 10, 2022

dramaticlly marked this pull request as ready for review January 10, 2022 20:18

dramaticlly force-pushed the master branch from ceca7be to 8f98e82 Compare January 11, 2022 20:55

Spark : Support parallelism in RemoveOrphanFiles

7551172

dramaticlly force-pushed the master branch from 8f98e82 to 7551172 Compare January 11, 2022 23:09

dramaticlly changed the title ~~Spark 3.2: Support parallelism in RemoveOrphanFiles~~ Spark : Support parallelism in RemoveOrphanFiles Jan 11, 2022

RussellSpitzer reviewed Jan 13, 2022

View reviewed changes

...4/spark/src/main/java/org/apache/iceberg/spark/actions/BaseDeleteOrphanFilesSparkAction.java Show resolved Hide resolved

RussellSpitzer reviewed Jan 13, 2022

View reviewed changes

...4/spark/src/main/java/org/apache/iceberg/spark/actions/BaseDeleteOrphanFilesSparkAction.java Show resolved Hide resolved

RussellSpitzer reviewed Jan 13, 2022

View reviewed changes

Add missing newlines and address PR feedback

22a411b

dramaticlly requested a review from RussellSpitzer January 13, 2022 16:45

szehon-ho approved these changes Jan 14, 2022

View reviewed changes

RussellSpitzer merged commit 7249d67 into apache:master Jan 14, 2022

findinpath mentioned this pull request Feb 9, 2022

Expire Snapshot and Remove Orphan files for Iceberg together with coordinator-only execute trinodb/trino#10810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spark : Support parallelism in RemoveOrphanFiles #3872

Spark : Support parallelism in RemoveOrphanFiles #3872

dramaticlly commented Jan 10, 2022 •

edited

dramaticlly commented Jan 10, 2022

szehon-ho commented Jan 10, 2022

RussellSpitzer commented Jan 11, 2022

dramaticlly commented Jan 11, 2022 •

edited

dramaticlly commented Jan 11, 2022

RussellSpitzer commented Jan 12, 2022

dramaticlly commented Jan 12, 2022

RussellSpitzer Jan 13, 2022

RussellSpitzer Jan 13, 2022

RussellSpitzer left a comment

dramaticlly commented Jan 13, 2022

RussellSpitzer commented Jan 13, 2022

dramaticlly commented Jan 13, 2022 •

edited

RussellSpitzer commented Jan 13, 2022

szehon-ho left a comment

szehon-ho Jan 14, 2022 •

edited

RussellSpitzer commented Jan 14, 2022

Spark : Support parallelism in RemoveOrphanFiles #3872

Spark : Support parallelism in RemoveOrphanFiles #3872

Conversation

dramaticlly commented Jan 10, 2022 • edited

Notes

Tests

dramaticlly commented Jan 10, 2022

szehon-ho commented Jan 10, 2022

RussellSpitzer commented Jan 11, 2022

dramaticlly commented Jan 11, 2022 • edited

dramaticlly commented Jan 11, 2022

RussellSpitzer commented Jan 12, 2022

dramaticlly commented Jan 12, 2022

RussellSpitzer Jan 13, 2022

Choose a reason for hiding this comment

RussellSpitzer Jan 13, 2022

Choose a reason for hiding this comment

RussellSpitzer left a comment

Choose a reason for hiding this comment

dramaticlly commented Jan 13, 2022

RussellSpitzer commented Jan 13, 2022

dramaticlly commented Jan 13, 2022 • edited

RussellSpitzer commented Jan 13, 2022

szehon-ho left a comment

Choose a reason for hiding this comment

szehon-ho Jan 14, 2022 • edited

Choose a reason for hiding this comment

RussellSpitzer commented Jan 14, 2022

dramaticlly commented Jan 10, 2022 •

edited

dramaticlly commented Jan 11, 2022 •

edited

dramaticlly commented Jan 13, 2022 •

edited

szehon-ho Jan 14, 2022 •

edited