[HUDI-332]Add operation type (insert/upsert/bulkinsert/delete) to HoodieCommitMetadata #1157

hddong · 2019-12-30T08:26:23Z

What is the purpose of the pull request

Add operation type (insert/upsert/bulkinsert/delete) to HoodieCommitMetadata

Brief change log

Add operation type (insert/upsert/bulkinsert/delete) to HoodieCommitMetadata

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.

hmatu · 2019-12-30T09:14:38Z

@hddong can you explain why add operation type to HoodieCommitMetadata? thanks

bvaradar

@hddong : Thanks for taking this task. Have added few comments. Looks to be going in correct direction.

bvaradar · 2019-12-31T01:01:34Z

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

@@ -171,7 +172,7 @@ public static SparkConf registerClasses(SparkConf conf) {
      JavaRDD<HoodieRecord<T>> taggedRecords = index.tagLocation(dedupedRecords, jsc, table);
      metrics.updateIndexMetrics(LOOKUP_STR, metrics.getDurationInMs(indexTimer == null ? 0L : indexTimer.stop()));
      indexTimer = null;
-      return upsertRecordsInternal(taggedRecords, commitTime, table, true);
+      return upsertRecordsInternal(taggedRecords, commitTime, table, true, Type.UPSERT);


There is an enum OperationType in HoodieWriteClient. It is more fine-grained in that it is able to distinguish between PREPPED and non-PREDDED version of operations. Can we use that enum instead of Type. You can move it to a separate enum class in the package org.apache.hudi.common.model and name it as WriteOperationType

bvaradar · 2019-12-31T01:04:38Z

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

@@ -106,6 +150,14 @@ public void setCompacted(Boolean compacted) {
    return filePaths;
  }

+  public void setOperateType(Type type) {


We also need to add the enum type to the avro schema hudi-common/src/main/avro/HoodieCommitMetadata.avsc

For archiving, we use the avro class org.apache.hudi.avro.model.HoodieCommitMetadata instead of org.apache.hudi.common.model.HoodieCommitMetadata

HoodieCommitArchiveLog.commitMetadataConverter needs to change to copy the operation types to avro objects.

bvaradar · 2019-12-31T01:07:30Z

hudi-common/src/test/java/org/apache/hudi/common/model/TestHoodieCommitMetadata.java

@@ -46,4 +47,25 @@ public void testPerfStatPresenceInHoodieMetadata() throws Exception {
    Assert.assertTrue(metadata.getTotalScanTime() == 0);
    Assert.assertTrue(metadata.getTotalLogFilesCompacted() > 0);
  }
+
+  @Test
+  public void testCompatibilityWithoutOperateType() throws Exception {


Thanks for adding the compatibility tests.

hddong · 2019-12-31T07:33:57Z

@hddong can you explain why add operation type to HoodieCommitMetadata? thanks

@hmatu It has no special function except for information tagging and tracing at this patch now.

hddong · 2019-12-31T07:45:10Z

@bvaradar thanks for your review and suggestion, I will modify later.

hddong · 2020-01-02T09:50:09Z

@bvaradar please re-review this.

bvaradar

@hddong : Just finished the review with some comments. Thanks for the contribution.

bvaradar · 2020-01-03T07:56:26Z

hudi-common/src/main/avro/HoodieCommitMetadata.avsc

@@ -129,6 +129,11 @@
         }],
         "default": null
      },
+      {
+         "name":"operateType",


nit: operationtionType

bvaradar · 2020-01-03T07:56:56Z

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

@@ -106,6 +108,14 @@ public void setCompacted(Boolean compacted) {
    return filePaths;
  }

+  public void setOperateType(WriteOperationType type) {


rename to operationType along with getters/setters.

bvaradar · 2020-01-03T07:58:46Z

hudi-spark/src/main/scala/org/apache/hudi/DataSourceOptions.scala

-  val INSERT_OPERATION_OPT_VAL = "insert"
-  val UPSERT_OPERATION_OPT_VAL = "upsert"
-  val DELETE_OPERATION_OPT_VAL = "delete"
+  val BULK_INSERT_OPERATION_OPT_VAL = WriteOperationType.BULK_INSERT.toString


Let us not change these configuration values as it would cause backwards compatibility issues.

bvaradar · 2020-01-03T08:03:09Z

hudi-common/src/main/avro/HoodieCommitMetadata.avsc

@@ -129,6 +129,11 @@
         }],
         "default": null
      },
+      {
+         "name":"operateType",
+         "type":["null","string"],


Can we use enum instead of string type ?

bvaradar · 2020-01-03T08:04:08Z

hudi-common/src/main/avro/HoodieCommitMetadata.avsc

+      {
+         "name":"operateType",
+         "type":["null","string"],
+         "default": null


Also, Can you confirm if the operation type is stored in the avro objects when archiving ?

bvaradar · 2020-01-03T08:07:01Z

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

@@ -510,21 +515,21 @@ private Partitioner getPartitioner(HoodieTable table, boolean isUpsert, Workload
  /**
   * Commit changes performed at the given commitTime marker.
   */
-  public boolean commit(String commitTime, JavaRDD<WriteStatus> writeStatuses) {
-    return commit(commitTime, writeStatuses, Option.empty());
+  public boolean commit(String commitTime, JavaRDD<WriteStatus> writeStatuses, WriteOperationType operationType) {


As only one hudi write operation is outstanding at a time, can you cache the last operation type in instance variables within HoodieWriteClient object so that users don't need to explicitly pass them in this commit() call

hddong · 2020-01-07T10:08:06Z

@bvaradar Operation type is stored in the avro objects when archiving, but there are a error here, it throw ClassCastException: org.apache.avro.generic.GenericData$EnumSymbol cannot be cast to org.apache.hudi.avro.model.WriteOperationType with deepCopy block in 'show archived commit stats' . Can you give any suggestion when you're free.

hddong · 2020-01-08T07:15:42Z

@bvaradar Operation type is stored in the avro objects when archiving, but there are a error here, it throw ClassCastException: org.apache.avro.generic.GenericData$EnumSymbol cannot be cast to org.apache.hudi.avro.model.WriteOperationType with deepCopy block in 'show archived commit stats' . Can you give any suggestion when you're free.

@bvaradar I found it cause by AVRO-1676 and fixed in AVRO-1.8.0. So can i roll back operationType to String type? It may cause other compatibility problem If upgrade avro.

hddong · 2020-01-10T01:21:21Z

@bvaradar All has been fixed, please review again. And operationType now be String, due to AVRO-1676 .

hmatu · 2020-01-10T01:44:24Z

IMO, WriteOperationType duplicate to OperationType, keep one is a better way.

bvaradar

@hddong : I am ok with the string type due to the enum deep copying issue. Once you address other comments, we can merge.

bvaradar · 2020-01-10T04:33:44Z

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

@@ -98,7 +99,7 @@
  private final transient HoodieMetrics metrics;
  private final transient HoodieCleanClient<T> cleanClient;
  private transient Timer.Context compactionTimer;
-
+  private transient WriteOperationType operationType;


Can you move this AbstractHoodieWriteClient and use setters/getters in HoodieWriteClient

bvaradar · 2020-01-10T04:35:50Z

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

@@ -492,6 +501,11 @@ protected void postCommit(HoodieCommitMetadata metadata, String instantTime,
    }
  }

+  @Override
+  protected void updateOperationType(HoodieCommitMetadata metadata) {


If we move the operationType instance to base-class, you no longer need this overridden method.

bvaradar · 2020-01-10T04:36:41Z

hudi-client/src/test/java/org/apache/hudi/io/TestHoodieCommitArchiveLog.java

@@ -397,10 +402,81 @@ public void testArchiveCommitCompactionNoHole() throws IOException {
        timeline.containsInstant(new HoodieInstant(false, HoodieTimeline.COMMIT_ACTION, "107")));
  }

+  @Test
+  public void testArchiveCommitAndDeepCopy() throws IOException {


Is this method used to test the enum issue in Avro? If so, you can remove it.

Is this method used to test the enum issue in Avro? If so, you can remove it.

Yes, be removed.

bvaradar · 2020-01-10T04:37:25Z

hudi-client/src/test/java/org/apache/hudi/io/TestHoodieCommitArchiveLog.java

+          (org.apache.hudi.avro.model.HoodieCommitMetadata) commitMetadataConverter.invoke(archiveLog, hoodieCommitMetadata);
+      assertEquals(expectedCommitMetadata.getOperationType(), WriteOperationType.INSERT.toString());
+    } catch (NoSuchMethodException e) {
+      e.printStackTrace();


Instead of e.printStackTrace() in all these catch blocks, can you throw these exception for the test to fail

hddong · 2020-01-10T08:00:58Z

@bvaradar Thanks very much for your review, all of them be addressed.

vinothchandar

Have some concerns on backwards compatibility.. We don't write commit metadata as avro , so may be this works for now?

vinothchandar · 2020-01-12T23:52:10Z

hudi-common/src/main/avro/HoodieCommitMetadata.avsc

@@ -129,6 +129,11 @@
         }],
         "default": null
      },
+      {


can we add this at the end? is nt that need for this to be backwards compatible?

is nt that need for this to be backwards compatible

Move it to the end. IMO, it has no impact on compatibility.

Agree with Vinoth, Avro schema evolution expects new fields to be appended.

hddong · 2020-01-13T01:34:21Z

Have some concerns on backwards compatibility.. We don't write commit metadata as avro , so may be this works for now?

@vinothchandar It works for archiving now, no for write commit metadata as avro.

vinothchandar · 2020-01-14T06:39:10Z

@hddong thanks! avro works based on field positions, so reordering them was my concern. Thanks for addressing this. over to @bvaradar

bvaradar · 2020-01-14T18:28:13Z

@hddong : lgtm. Once you rebase this PR and resolve conflicts, we can merge

hddong · 2020-01-14T23:44:42Z

@bvaradar @vinothchandar Thanks. rebased this and there are no conflicts now.

bvaradar · 2020-01-15T00:35:07Z

@hddong : Did you forgot to push the diff. Still seeing conflicts.

hddong · 2020-01-16T01:33:36Z

@bvaradar this was weird. In my side, this web page shows there are no conflicts, it's all passed and ready to merge.

bvaradar · 2020-01-16T01:59:50Z

bvaradar · 2020-01-16T02:00:56Z

@hddong : If possible try to see if you can run it again ? pull master, rebase and force-push

hddong · 2020-01-16T07:32:56Z

@bvaradar I tried what you suggest, but there are some other problems. I tried another way and tried rebase locally, it seems ok.

hddong · 2020-01-31T09:20:24Z

@bvaradar @vinothchandar please review this again.

hmatu · 2020-01-31T11:16:17Z

@bvaradar Operation type is stored in the avro objects when archiving, but there are a error here, it throw ClassCastException: org.apache.avro.generic.GenericData$EnumSymbol cannot be cast to org.apache.hudi.avro.model.WriteOperationType with deepCopy block in 'show archived commit stats' . Can you give any suggestion when you're free.

@bvaradar I found it cause by AVRO-1676 and fixed in AVRO-1.8.0. So can i roll back operationType to String type? It may cause other compatibility problem If upgrade avro.

Already using avro-1.8.2 dependency, String type is redundant now, Enum type is enough.

https://github.com/apache/incubator-hudi/blob/f27c7a16c6d437efaa83e50a7117b83e5201ac49/pom.xml#L96

bvaradar · 2020-02-26T19:22:49Z

@hmatu : Sorry for the long delay. Was offline for a while. I agree that operationType should be string in both avro and json structure to keep it simple. Once you make the final change and resolve merge conflict, I will merge this PR.

hddong · 2020-03-02T14:37:20Z

@bvaradar As you said keep operationType string here, and I had resolve all conflict.

bvaradar · 2020-03-02T22:55:15Z

hudi-client/src/test/java/org/apache/hudi/io/TestHoodieCommitArchiveLog.java

+
+    Class<?> clazz  = HoodieCommitArchiveLog.class;
+    try {
+      Method commitMetadataConverter = clazz.getDeclaredMethod("commitMetadataConverter", HoodieCommitMetadata.class);


@hddong One final comment : Can you make commitMetadataConverter() in HoodieCommitArchiveLog with default access (instead of private). This way, you dont need to deal with reflection.

@hddong One final comment : Can you make commitMetadataConverter() in HoodieCommitArchiveLog with default access (instead of private). This way, you dont need to deal with reflection.

@bvaradar Change to publice access, because TestHoodieCommitArchiveLog and HoodieCommitArchiveLog not in the same package path.

bvaradar

One minor comment. Otherwise ready to merge.

codecov-io · 2020-03-03T09:30:19Z

Codecov Report

Merging #1157 into master will decrease coverage by 0.09%.
The diff coverage is 80%.

@@             Coverage Diff             @@
##             master    #1157     +/-   ##
===========================================
- Coverage     67.09%   66.99%   -0.1%     
  Complexity      223      223             
===========================================
  Files           333      334      +1     
  Lines         16216    16269     +53     
  Branches       1659     1660      +1     
===========================================
+ Hits          10880    10900     +20     
- Misses         4598     4632     +34     
+ Partials        738      737      -1

Impacted Files	Coverage Δ	Complexity Δ
.../org/apache/hudi/table/HoodieCommitArchiveLog.java	`75% <ø> (ø)`	`0 <0> (ø)`	⬇️
.../apache/hudi/client/AbstractHoodieWriteClient.java	`70.89% <100%> (-4.24%)`	`0 <0> (ø)`
...apache/hudi/common/model/HoodieCommitMetadata.java	`54.58% <100%> (-3.5%)`	`0 <0> (ø)`
...java/org/apache/hudi/client/HoodieWriteClient.java	`69.77% <100%> (+0.54%)`	`0 <0> (ø)`	⬇️
...g/apache/hudi/common/model/WriteOperationType.java	`57.14% <57.14%> (ø)`	`0 <0> (?)`
...n/java/org/apache/hudi/common/model/HoodieKey.java	`94.44% <0%> (+5.55%)`	`0% <0%> (ø)`	⬇️

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 2d04014...e34024f. Read the comment docs.

bvaradar

Looks great. Thanks for the patience @hddong

…dieCommitMetadata (apache#1157) [HUDI-332]Add operation type (insert/upsert/bulkinsert/delete) to HoodieCommitMetadata (apache#1157)

hddong changed the title ~~Add operation type (insert/upsert/bulkinsert/delete) to HoodieCommitMetadata~~ [HUDI-332]Add operation type (insert/upsert/bulkinsert/delete) to HoodieCommitMetadata Dec 30, 2019

bvaradar requested changes Dec 31, 2019

View reviewed changes

bvaradar self-assigned this Dec 31, 2019

hddong requested a review from bvaradar January 2, 2020 09:49

bvaradar reviewed Jan 3, 2020

View reviewed changes

bvaradar approved these changes Jan 10, 2020

View reviewed changes

vinothchandar reviewed Jan 12, 2020

View reviewed changes

hddong force-pushed the hudi-332-add-operation-type branch 2 times, most recently from f048357 to 68e61ab Compare January 16, 2020 06:55

vinothchandar assigned n3nash Feb 5, 2020

hddong force-pushed the hudi-332-add-operation-type branch from 759cde9 to 68e61ab Compare March 2, 2020 12:43

Rebase

9ad174a

hddong force-pushed the hudi-332-add-operation-type branch from 68e61ab to 9ad174a Compare March 2, 2020 12:54

Remove unused import

3a25a75

bvaradar reviewed Mar 2, 2020

View reviewed changes

bvaradar approved these changes Mar 2, 2020

View reviewed changes

hddong added 3 commits March 3, 2020 16:30

Update TestHoodieCommitArchiveLog.java

02d5e52

Update HoodieCommitArchiveLog.java

8fd88f6

Update TestHoodieCommitArchiveLog.java

e34024f

bvaradar approved these changes Mar 3, 2020

View reviewed changes

bvaradar merged commit 8306205 into apache:master Mar 3, 2020

[HUDI-332]Add operation type (insert/upsert/bulkinsert/delete) to HoodieCommitMetadata #1157

[HUDI-332]Add operation type (insert/upsert/bulkinsert/delete) to HoodieCommitMetadata #1157

Conversation

hddong commented Dec 30, 2019 • edited

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

hmatu commented Dec 30, 2019

bvaradar 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

hddong commented Dec 31, 2019

hddong commented Dec 31, 2019

hddong commented Jan 2, 2020

bvaradar 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

hddong commented Jan 7, 2020

hddong commented Jan 8, 2020 • edited

hddong commented Jan 10, 2020 • edited

hmatu commented Jan 10, 2020

bvaradar 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

hddong commented Jan 10, 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

hddong commented Jan 13, 2020

vinothchandar commented Jan 14, 2020

bvaradar commented Jan 14, 2020

hddong commented Jan 14, 2020

bvaradar commented Jan 15, 2020

hddong commented Jan 16, 2020

bvaradar commented Jan 16, 2020

bvaradar commented Jan 16, 2020

hddong commented Jan 16, 2020

hddong commented Jan 31, 2020

hmatu commented Jan 31, 2020 • edited

bvaradar commented Feb 26, 2020

hddong commented Mar 2, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaradar left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 3, 2020

Codecov Report

bvaradar left a comment

Choose a reason for hiding this comment

hddong commented Dec 30, 2019 •

edited

hddong commented Jan 8, 2020 •

edited

hddong commented Jan 10, 2020 •

edited

hmatu commented Jan 31, 2020 •

edited

hddong commented Mar 2, 2020 •

edited