[HUDI-770] Organize upsert/insert API implementation under a single package #1495

bvaradar · 2020-04-08T04:00:07Z

[HUDI-770] Organize upsert/insert API implementation under a single package

Created ActionExecutor for performing upsert/insert.

There is inherent complexity with supporting auto commit vs non-auto commit. Once that is resolved, we will be able to remove much of the code in HoodieWriteClient.

codecov-io · 2020-04-08T05:35:55Z

Codecov Report

Merging #1495 into master will decrease coverage by 0.07%.
The diff coverage is 81.48%.

@@             Coverage Diff              @@
##             master    #1495      +/-   ##
============================================
- Coverage     72.25%   72.17%   -0.08%     
  Complexity      289      289              
============================================
  Files           338      365      +27     
  Lines         15946    16228     +282     
  Branches       1624     1632       +8     
============================================
+ Hits          11521    11712     +191     
- Misses         3697     3783      +86     
- Partials        728      733       +5

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/org/apache/hudi/table/HoodieTable.java	`79.64% <ø> (ø)`	`0.00 <0.00> (ø)`
...g/apache/hudi/table/action/BaseActionExecutor.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...ltacommit/BulkInsertDeltaCommitActionExecutor.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...it/BulkInsertPreppedDeltaCommitActionExecutor.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...commit/InsertPreppedDeltaCommitActionExecutor.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...commit/UpsertPreppedDeltaCommitActionExecutor.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
.../org/apache/hudi/table/HoodieMergeOnReadTable.java	`73.33% <22.22%> (-9.80%)`	`0.00 <0.00> (ø)`
.../action/commit/BulkInsertCommitActionExecutor.java	`55.55% <55.55%> (ø)`	`0.00 <0.00> (?)`
.../commit/BulkInsertPreppedCommitActionExecutor.java	`55.55% <55.55%> (ø)`	`0.00 <0.00> (?)`
.../apache/hudi/table/action/commit/DeleteHelper.java	`64.28% <64.28%> (ø)`	`0.00 <0.00> (?)`
... and 60 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 c0f96e0...8a3b612. Read the comment docs.

vinothchandar

Few suggestions!

vinothchandar · 2020-04-09T04:45:49Z

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

-    } else {
-      return table.getInsertPartitioner(profile, jsc);
+      HoodieTable<T> hoodieTable) {
+    CommitActionResult result = hoodieTable.ingest(jsc, preppedRecords, instantTime, getOperationType());


IMO we should just mirror the same APIs upsert, insert on the table.. ingest is confusing, since it also implies that we are reading from some source, whihc we are not..

vinothchandar · 2020-04-09T04:48:26Z

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

+    return true;
+  }
+
+  void doPostCommitAndEmitCommitMetrics(String instantTime, HoodieCommitMetadata metadata,


break this into two methods? one to do the post commit and one to emit?

vinothchandar · 2020-04-09T04:49:08Z

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

-    } else {
-      return table.getInsertPartitioner(profile, jsc);
+      HoodieTable<T> hoodieTable) {
+    CommitActionResult result = hoodieTable.ingest(jsc, preppedRecords, instantTime, getOperationType());


and can we return HoodieCommitMetadata instead? I guess the issue is we have to pass this to the caller (deltastreamer or datasource) to decide whether to commit or not?

Yes, Supporting externally triggered commit is the reason why we have CommitActionResult.

vinothchandar · 2020-04-09T04:51:58Z

...ient/src/main/java/org/apache/hudi/table/action/commit/AbstractBaseCommitActionExecutor.java

+
+import scala.Tuple2;
+
+public abstract class AbstractBaseCommitActionExecutor<T extends HoodieRecordPayload<T>>


rename to just BaseCommitActionExecutor?

vinothchandar · 2020-04-09T04:54:37Z

hudi-client/src/main/java/org/apache/hudi/table/action/commit/CommitActionResult.java

+public class CommitActionResult {
+
+  private JavaRDD<WriteStatus> writeStatuses;
+  private Duration indexUpdateDuration;


if tagging the index is outside this action, then updating should be moved out as well?

This is fixed as as part of creating separate executors for each operation.

vinothchandar · 2020-04-09T04:57:59Z

hudi-common/src/main/java/org/apache/hudi/common/model/WriteOperationType.java


-  WriteOperationType(String value) {
+  WriteOperationType(String value, boolean isUpsert) {


is nt this really debt? can we get rid of this boolean and just replace with a helper method that takes in the operation type and returns true/false?

vinothchandar · 2020-04-09T04:58:33Z

hudi-common/src/main/java/org/apache/hudi/common/model/WriteOperationType.java

@@ -69,4 +71,8 @@ public static WriteOperationType fromValue(String value) {
        throw new HoodieException("Invalid value of Type.");
    }
  }
+
+  public boolean isUpsert() {


Like this method, can be just a static method that returns true/false

vinothchandar · 2020-04-09T05:00:55Z

hudi-client/src/main/java/org/apache/hudi/table/action/commit/CommitActionResult.java

+/**
+ * Contains metadata, write-statuses and latency times corresponding to a commit/delta-commit action.
+ */
+public class CommitActionResult {


Ideally we are able to return HoodieCommitMetadata... but since we can't, may be still rename this to HoodieWriteMetadata to stay consistent with the other objects we are returning now. (RollbackMetadata, RestoreMetadata etc)?

bvaradar · 2020-04-12T23:49:43Z

@vinothchandar : Addressed review comments. Please take a look when you get a chance.

vinothchandar

Left some comments on naming.. Overall, felt we can organize for reuse more.. but this already improves things substantially.. So please land after you go over the comments and CI is happy

hudi-client/src/main/java/org/apache/hudi/table/action/commit/DedupeWriteHelper.java

...-client/src/main/java/org/apache/hudi/table/action/commit/MergeOnReadBulkInsertExecutor.java

...lient/src/main/java/org/apache/hudi/table/action/commit/MergeOnReadCommitActionExecutor.java

hudi-client/src/main/java/org/apache/hudi/table/action/commit/MergeOnReadDeleteExecutor.java

hudi-common/src/main/java/org/apache/hudi/common/model/WriteOperationType.java

…ackage

bvaradar · 2020-04-13T05:08:39Z

@vinothchandar : Addressed comments. Will merge once the CI passes.

vinothchandar self-assigned this Apr 8, 2020

vinothchandar reviewed Apr 9, 2020

View reviewed changes

bvaradar force-pushed the hudi-408 branch 2 times, most recently from 5359fd0 to ce8cc69 Compare April 12, 2020 23:05

vinothchandar approved these changes Apr 13, 2020

View reviewed changes

[HUDI-770] Organize upsert/insert API implementation under a single p…

8a3b612

…ackage

bvaradar force-pushed the hudi-408 branch from ce8cc69 to 8a3b612 Compare April 13, 2020 05:07

bvaradar merged commit 17bf930 into apache:master Apr 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-770] Organize upsert/insert API implementation under a single package #1495

[HUDI-770] Organize upsert/insert API implementation under a single package #1495

bvaradar commented Apr 8, 2020 •

edited

codecov-io commented Apr 8, 2020 •

edited

vinothchandar left a comment

vinothchandar Apr 9, 2020

bvaradar Apr 12, 2020

vinothchandar Apr 9, 2020

bvaradar Apr 12, 2020

vinothchandar Apr 9, 2020

bvaradar Apr 12, 2020

vinothchandar Apr 9, 2020

bvaradar Apr 12, 2020

vinothchandar Apr 9, 2020

bvaradar Apr 12, 2020

vinothchandar Apr 9, 2020

bvaradar Apr 12, 2020

vinothchandar Apr 9, 2020

bvaradar Apr 12, 2020

vinothchandar Apr 9, 2020

bvaradar Apr 12, 2020

bvaradar commented Apr 12, 2020

vinothchandar left a comment

bvaradar commented Apr 13, 2020


		import scala.Tuple2;

		public abstract class AbstractBaseCommitActionExecutor<T extends HoodieRecordPayload<T>>


		WriteOperationType(String value) {
		WriteOperationType(String value, boolean isUpsert) {

[HUDI-770] Organize upsert/insert API implementation under a single package #1495

[HUDI-770] Organize upsert/insert API implementation under a single package #1495

Conversation

bvaradar commented Apr 8, 2020 • edited

codecov-io commented Apr 8, 2020 • edited

Codecov Report

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

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

Choose a reason for hiding this comment

bvaradar commented Apr 12, 2020

vinothchandar left a comment

Choose a reason for hiding this comment

bvaradar commented Apr 13, 2020

bvaradar commented Apr 8, 2020 •

edited

codecov-io commented Apr 8, 2020 •

edited