[HUDI-679] Make io package Spark free #1460

leesf · 2020-03-28T06:36:29Z

Tips

Thank you very much for contributing to Apache Hudi.
Please review https://hudi.apache.org/contributing.html before opening a pull request.

What is the purpose of the pull request

Make io package spark free.

Brief change log

Introduce SparkSupplier.java
Remove the usage of TaskContext in io package.

Verify this pull request

This pull request is a trivial rework / code cleanup without any test coverage.

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

leesf · 2020-03-28T06:37:18Z

@vinothchandar @yanghua Please take a look when you are free. Thanks

codecov-io · 2020-03-28T10:32:08Z

Codecov Report

Merging #1460 into master will decrease coverage by 0.08%.
The diff coverage is 97.36%.

@@             Coverage Diff              @@
##             master    #1460      +/-   ##
============================================
- Coverage     67.66%   67.58%   -0.09%     
  Complexity      261      261              
============================================
  Files           342      348       +6     
  Lines         16510    16670     +160     
  Branches       1684     1693       +9     
============================================
+ Hits          11172    11266      +94     
- Misses         4599     4665      +66     
  Partials        739      739

Impacted Files	Coverage Δ	Complexity Δ
.../hudi/execution/MergeOnReadLazyInsertIterable.java	`64.70% <66.66%> (ø)`	`0.00 <0.00> (ø)`
...g/apache/hudi/client/SparkTaskContextSupplier.java	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
...g/apache/hudi/execution/BulkInsertMapFunction.java	`100.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`
.../hudi/execution/CopyOnWriteLazyInsertIterable.java	`80.76% <100.00%> (+0.37%)`	`0.00 <0.00> (ø)`
...in/java/org/apache/hudi/io/HoodieAppendHandle.java	`84.17% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...in/java/org/apache/hudi/io/HoodieCreateHandle.java	`84.61% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...ain/java/org/apache/hudi/io/HoodieMergeHandle.java	`79.31% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...ain/java/org/apache/hudi/io/HoodieWriteHandle.java	`75.00% <100.00%> (+1.66%)`	`0.00 <0.00> (ø)`
...rg/apache/hudi/io/storage/HoodieParquetWriter.java	`100.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...he/hudi/io/storage/HoodieStorageWriterFactory.java	`93.75% <100.00%> (ø)`	`0.00 <0.00> (ø)`
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1713f68...07f073a. Read the comment docs.

yanghua

@leesf I left some comments you can consider.

yanghua · 2020-03-28T11:01:54Z

hudi-client/src/main/java/org/apache/hudi/client/SparkSupplier.java

+/**
+ * Spark Supplier.
+ */
+public interface SparkSupplier<T> extends Supplier<T>, Serializable {


Considering this interface supports some information about TaskContext, can we rename to SparkTaskContextDetailSupplier?

sounds reasonable.

yanghua · 2020-03-28T11:02:47Z

hudi-client/src/main/java/org/apache/hudi/client/SparkSupplier.java

+ * Spark Supplier.
+ */
+public interface SparkSupplier<T> extends Supplier<T>, Serializable {
+  SparkSupplier<Integer> PARTITION_SUPPLIER = () -> TaskContext.getPartitionId();


Can we add empty line to split this definition? Additionally, add some comments?

Can we add empty line to split this definition? Additionally, add some comments?

sure

leesf · 2020-03-28T11:32:00Z

@yanghua Updated this PR to address your comments.

yanghua

Thanks for addressing those comments. Left some new comments.

yanghua · 2020-03-28T13:02:40Z

hudi-client/src/main/java/org/apache/hudi/client/SparkTaskContextDetailSupplier.java

+  /**
+   * Supplier to get partition id.
+   */
+  SparkTaskContextDetailSupplier<Integer> PARTITION_SUPPLIER = () -> TaskContext.getPartitionId();


PARTITION_ID_SUPPLIER ?

yanghua · 2020-03-28T13:03:01Z

hudi-client/src/main/java/org/apache/hudi/client/SparkTaskContextDetailSupplier.java

+  /**
+   * Supplier to get stage id.
+   */
+  SparkTaskContextDetailSupplier<Integer> STAGE_SUPPLIER = () -> TaskContext.get().stageId();


STAGE_ID_SUPPLIER ?

yanghua · 2020-03-28T13:03:29Z

hudi-client/src/main/java/org/apache/hudi/client/SparkTaskContextDetailSupplier.java

+  /**
+   * Supplier to get task attempt id.
+   */
+  SparkTaskContextDetailSupplier<Long> ATTEMPT_SUPPLIER = () -> TaskContext.get().taskAttemptId();


ATTEMPT_ID_SUPPLIER ?

yanghua · 2020-03-28T13:05:15Z

hudi-client/src/main/java/org/apache/hudi/execution/BulkInsertMapFunction.java

@@ -51,6 +51,7 @@ public BulkInsertMapFunction(String instantTime, HoodieWriteConfig config, Hoodi
  @Override
  public Iterator<List<WriteStatus>> call(Integer partition, Iterator<HoodieRecord<T>> sortedRecordItr) {
    return new CopyOnWriteLazyInsertIterable<>(sortedRecordItr, config, instantTime, hoodieTable,
-        fileIDPrefixes.get(partition));
+        fileIDPrefixes.get(partition), hoodieTable.getIdSupplier(), hoodieTable.getStageSupplier(),


hoodieTable.getIdSupplier() is not clear here. I suggest we can rename these getter to e.g. getPartitionIdSupplier, getStageId and getAttemptId?

yanghua · 2020-03-28T13:05:33Z

hudi-client/src/main/java/org/apache/hudi/execution/CopyOnWriteLazyInsertIterable.java

@@ -50,15 +51,23 @@
  protected final HoodieTable<T> hoodieTable;
  protected final String idPrefix;
  protected int numFilesWritten;
+  protected Supplier<Integer> idSupplier;


yanghua · 2020-03-28T13:06:49Z

hudi-client/src/main/java/org/apache/hudi/execution/CopyOnWriteLazyInsertIterable.java

-                                       String instantTime, HoodieTable<T> hoodieTable, String idPrefix) {
+                                       String instantTime, HoodieTable<T> hoodieTable, String idPrefix,
+                                       Supplier<Integer> idSupplier, Supplier<Integer> stageSupplier,
+                                       Supplier<Long> attemptSupplier) {


Actually, I am not sure if we can package these three args into a DTO structure. Just a thought, you can ignore.

Actually, I am not sure if we can package these three args into a DTO structure. Just a thought, you can ignore.

Yes, I think it is better.

yanghua · 2020-03-28T13:08:31Z

hudi-client/src/main/java/org/apache/hudi/io/HoodieWriteHandle.java

@@ -55,26 +55,32 @@
  protected final String partitionPath;
  protected final String fileId;
  protected final String writeToken;
+  protected final Supplier<Integer> idSupplier;


ditto, IMO, id here is not clear. partition id is better.

leesf · 2020-03-28T15:24:53Z

@yanghua Updated this PR to package three suppliers into Suppliers. PTAL, thanks.

vinothchandar

Would love to eliminate the Suppliers abstraction :) .. we can just have an interface the serves up three values and pass that in

vinothchandar · 2020-03-28T19:24:56Z

hudi-client/src/main/java/org/apache/hudi/client/SparkTaskContextDetailSupplier.java

+/**
+ * Spark Supplier.
+ */
+public interface SparkTaskContextDetailSupplier<T> extends Supplier<T>, Serializable {


Should be interface be something generic like WriteTaskContextSupplier which is extended by SparkTaskContextSupplier ?

vinothchandar · 2020-03-28T19:27:59Z

hudi-client/src/main/java/org/apache/hudi/client/SparkTaskContextDetailSupplier.java

+/**
+ * Spark Supplier.
+ */
+public interface SparkTaskContextDetailSupplier<T> extends Supplier<T>, Serializable {


I am not sure this abstraction is at the right level.. Should this have to be Supplier.. I think we can just have three methods that return Supplier<Integer> and Supplier<Long> and pass just one argument through the code path i.e the SparkTaskContextSuppler instance..

Just online seeing the latest changes comes from @leesf . Yes, it seems this is a better abstraction.

vinothchandar · 2020-03-28T19:29:06Z

hudi-client/src/main/java/org/apache/hudi/client/Suppliers.java

+/**
+ * A bundle of Suppliers.
+ */
+public class Suppliers implements Serializable {


then this can go away.. I feel this is additional abstraction, that we may not need..

vinothchandar · 2020-03-28T19:30:23Z

hudi-client/src/main/java/org/apache/hudi/execution/MergeOnReadLazyInsertIterable.java

@@ -35,8 +36,9 @@
 public class MergeOnReadLazyInsertIterable<T extends HoodieRecordPayload> extends CopyOnWriteLazyInsertIterable<T> {

  public MergeOnReadLazyInsertIterable(Iterator<HoodieRecord<T>> sortedRecordItr, HoodieWriteConfig config,
-      String instantTime, HoodieTable<T> hoodieTable, String idPfx) {
-    super(sortedRecordItr, config, instantTime, hoodieTable, idPfx);
+                                       String instantTime, HoodieTable<T> hoodieTable, String idPfx,


side point: we should fix method arg formatting consistently between intellij and checkstyle. Keep seeing these sort of whitespace changes in PRs.

vinothchandar · 2020-03-28T19:31:33Z

hudi-client/src/main/java/org/apache/hudi/io/HoodieWriteHandle.java

    this.originalSchema = new Schema.Parser().parse(config.getSchema());
    this.writerSchema = createHoodieWriteSchema(originalSchema);
    this.timer = new HoodieTimer().startTimer();
    this.writeStatus = (WriteStatus) ReflectionUtils.loadClass(config.getWriteStatusClassName(),
        !hoodieTable.getIndex().isImplicitWithStorage(), config.getWriteStatusFailureFraction());
+    this.suppliers = suppliers;
+    this.writeToken = makeSparkWriteToken();


rename to just makeWriteToken()?

leesf · 2020-03-29T05:14:09Z

@yanghua @vinothchandar Thanks for your review, just updated the PR to address your comments. PTAL.

vinothchandar

LGTM

* [HUDI-679] Make io package Spark free

[HUDI-679] Make io package Spark free

5f65bfb

leesf requested review from vinothchandar and yanghua March 28, 2020 06:36

leesf added 2 commits March 28, 2020 16:31

address travis failure

24e68e3

add travis

bb7268f

yanghua reviewed Mar 28, 2020

View reviewed changes

address comments

9ef6c85

yanghua reviewed Mar 28, 2020

View reviewed changes

vinothchandar self-assigned this Mar 28, 2020

leesf added 2 commits March 28, 2020 23:21

address comments

934c41d

address comments

e9a3c06

vinothchandar requested changes Mar 28, 2020

View reviewed changes

leesf added 2 commits March 29, 2020 13:09

address comments

3d215b8

format

07f073a

vinothchandar approved these changes Mar 29, 2020

View reviewed changes

yanghua approved these changes Mar 29, 2020

View reviewed changes

yanghua merged commit 07c3c5d into apache:master Mar 29, 2020

leesf deleted the HUDI-679 branch March 29, 2020 09:36

lyogev pushed a commit to YotpoLtd/incubator-hudi that referenced this pull request Mar 30, 2020

[HUDI-679] Make io package Spark free (apache#1460)

7f94486

* [HUDI-679] Make io package Spark free

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-679] Make io package Spark free #1460

[HUDI-679] Make io package Spark free #1460

leesf commented Mar 28, 2020

leesf commented Mar 28, 2020

codecov-io commented Mar 28, 2020 •

edited

yanghua left a comment

yanghua Mar 28, 2020

leesf Mar 28, 2020

yanghua Mar 28, 2020

leesf Mar 28, 2020

leesf commented Mar 28, 2020

yanghua left a comment

yanghua Mar 28, 2020

yanghua Mar 28, 2020

yanghua Mar 28, 2020

yanghua Mar 28, 2020

yanghua Mar 28, 2020

yanghua Mar 28, 2020

leesf Mar 28, 2020

yanghua Mar 28, 2020

leesf commented Mar 28, 2020

vinothchandar left a comment

vinothchandar Mar 28, 2020

vinothchandar Mar 28, 2020

yanghua Mar 29, 2020

vinothchandar Mar 28, 2020

vinothchandar Mar 28, 2020

vinothchandar Mar 28, 2020

leesf commented Mar 29, 2020

vinothchandar left a comment

[HUDI-679] Make io package Spark free #1460

[HUDI-679] Make io package Spark free #1460

Conversation

leesf commented Mar 28, 2020

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

leesf commented Mar 28, 2020

codecov-io commented Mar 28, 2020 • edited

Codecov Report

yanghua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leesf commented Mar 28, 2020

yanghua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leesf commented Mar 28, 2020

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leesf commented Mar 29, 2020

vinothchandar left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 28, 2020 •

edited