Fix for updating duplicate records in same/different files in same pa… #380

suniluber · 2018-04-16T23:28:04Z

Fix for #333

@ovj @vinothchandar @jianxu @n3nash

CLAassistant · 2018-04-16T23:28:11Z

All committers have signed the CLA.

ovj · 2018-04-17T00:03:03Z

Also @bvaradar.

vinothchandar · 2018-04-17T21:58:14Z

@suniluber can you sign the CLA ?

suniluber · 2018-04-17T22:02:38Z

@vinothchandar signed the CLA.

vinothchandar · 2018-04-17T22:10:22Z

hoodie-client/src/main/java/com/uber/hoodie/index/bloom/HoodieBloomIndex.java

+          // If a record being tagged exists in multiple files in the same partition and if the
+          // currentLocation has already been set, then create a new record with the same key
+          // and data, set the current path of the new record and return it.
+          if (record.getCurrentLocation() != null) {


whats the intent of this if-block? Seems you are re-initing the same object again based on it self?

When you a record in multiple files in the same partition, then rowKeyRecordPairRDD will have 2 entries with the same exact in memory copy of the HoodieRecord and the 2 separate filenames that the record is found in. In this case, setCurrentLocation will fail for the second file, as we are setting the location on the same in memory object. HoodieRecord restricts setting the currentLocation if it is already set. So creating multiple copies of the HoodieRecord with the different file locations.

I can create a new HoodieRecord here and set the current location. Thoughts ?

I see. this is very hacky :) .. arghhh. :)
can you explain this more clearly in the comments there? esp this part "When you a record in multiple files in the same partition, then rowKeyRecordPairRDD will have 2 entries with the same exact in memory copy of the HoodieRecord and the 2 separate filenames that the record is found in"

we can leave as it is.

vinothchandar · 2018-04-17T22:12:40Z

hoodie-client/src/main/java/com/uber/hoodie/io/HoodieMergeHandle.java

@@ -185,8 +191,7 @@ private boolean writeUpdateRecord(HoodieRecord<T> hoodieRecord,
  }

  /**
-   * Go through an old record. Here if we detect a newer version shows up, we write the new one to
-   * the file.
+   * Go through an old record. Here if we detect a newer version shows up, we write the new one to the file.


can you please revert pure whitespace changes?

vinothchandar · 2018-04-17T22:14:07Z

hoodie-client/src/test/java/com/uber/hoodie/TestHoodieClientBase.java

@@ -107,6 +107,10 @@ protected HoodieWriteConfig getConfig() {
        .withIndexConfig(HoodieIndexConfig.newBuilder().withIndexType(HoodieIndex.IndexType.BLOOM).build());
  }

+  HoodieWriteConfig.Builder getConfigWithBulkInsertParallelismBuilder() {
+    return getConfigBuilder().withBulkInsertParallelism(1);


1 is very restrictive.. Can we make it atleast 2, so there is some notion of parallel compute

vinothchandar · 2018-04-17T22:21:31Z

hoodie-client/src/main/java/com/uber/hoodie/io/HoodieMergeHandle.java

@@ -52,6 +54,7 @@

  private WriteStatus writeStatus;
  private Map<String, HoodieRecord<T>> keyToNewRecords;
+  private Set<String> existingRecords;


rename to : writtenRecordKeys

vinothchandar · 2018-04-17T22:24:09Z

hoodie-client/src/test/java/com/uber/hoodie/TestHoodieClientOnCopyOnWriteStorage.java

@@ -581,6 +587,171 @@ public void testCommitWritesRelativePaths() throws Exception {
    }
  }

+  @Test
+  public void testUpsertsForMultipleRecordsInSameFile() throws Exception {


Can we just create a IOHandle level test for this? I would like to leave this file for API level tests.

vinothchandar · 2018-04-17T22:26:40Z

hoodie-client/src/test/java/com/uber/hoodie/common/HoodieTestDataGenerator.java

@@ -247,6 +258,16 @@ public HoodieRecord generateUpdateRecord(HoodieKey key, String commitTime) throw
    return updates;
  }

+  /**
+   * Generates 1 new update, given the hoodie key, riderName and driverName
+   */


can we call generateUpdates(commitTime, 1) ? Please try to reuse the existing methods as much as possible

vinothchandar · 2018-04-17T22:27:57Z

hoodie-client/src/test/java/com/uber/hoodie/common/HoodieTestDataGenerator.java

+  /**
+   * Generates a new avro record of the above schema format, with the given hoodiekey, riderName and driverName
+   */
+  public static TestRawTripPayload generateGenericRecord(HoodieKey key, String commitTime,


This is returning a TestRawTripPayload .. Can we reuse generateRandomValue() above.. any specific reason you need rider and driver names specifically passed in and not use the defaults there?

bvaradar

Agree with Vinoth's comments.

In general, I feel we should just put the guard in all our public APIs (HoodieWriteClient) to prevent these cases. An extra reduceByKey on incoming RDD will avoid the problem in first place and keep the data-set uncorrupted.

Even with the fix in this PR, there is no guarantee that readers will not be seeing duplicate records. @vinothchandar : Thoughts ?

…rition

suniluber · 2018-04-24T02:28:22Z

@vinothchandar I have made the discussed changes. Can you review when you get a chance ?

vinothchandar · 2018-04-25T15:01:56Z

@bvaradar i think sunil does not want to pay the reducebykey cost and thus the changes. I have clarified with them that they should have unit test in the ingestion system to keep ensuring behavior and this just happens to work :)

@suniluber will take a pass today

vinothchandar · 2018-04-25T16:35:27Z

hoodie-client/src/test/java/com/uber/hoodie/io/TestHoodieMergeHandleDuplicateRecords.java

+  }
+
+  @Test
+  public void testUpsertsForMultipleRecordsInSameFile() throws Exception {


@suniluber Like I mentioned before, can't we write a IOHandle level test for this?

is this test covering in a guaranteed way - duplicates inside same file and duplicates within same partition but different files?
Again I feel a lower level unit test would test all that.. Does such a test exist elsewhere for e.g in the ingestion code? I think those two are critical cases to cover to ensure this does not regress

@ovj @n3nash

@vinothchandar yes, this test is covering duplicates within the same file in the same partition and duplicates within different files in the same partition.

vinothchandar · 2018-04-25T16:37:53Z

hoodie-client/src/test/java/com/uber/hoodie/io/TestHoodieMergeHandleDuplicateRecords.java

+    }
+
+    Dataset<Row> dataSet = HoodieClientTestUtils.read(basePath, sqlContext, fs, fullPartitionPaths);
+


nit: please remove extra blank line

vinothchandar · 2018-04-25T16:38:44Z

hoodie-client/src/test/java/com/uber/hoodie/io/TestHoodieMergeHandleDuplicateRecords.java

+     */
+    newCommitTime = "003";
+    client.startCommitWithTime(newCommitTime);
+


can you please get rid of extra newlines in this file,

vinothchandar

Few comments. LG otherwise

…pache#380)

vinothchandar requested changes Apr 17, 2018

View reviewed changes

bvaradar reviewed Apr 21, 2018

View reviewed changes

suniluber force-pushed the updateduplicaterecords branch from 1491dd1 to c5f63fa Compare April 23, 2018 20:34

Fix for updating duplicate records in same/different files in same pa…

bdd77dc

…rition

suniluber force-pushed the updateduplicaterecords branch from c5f63fa to bdd77dc Compare April 23, 2018 22:23

vinothchandar reviewed Apr 25, 2018

View reviewed changes

Added more comments and removed the extra new lines

efdfa44

vinothchandar approved these changes Apr 25, 2018

View reviewed changes

vinothchandar merged commit b9b9b24 into apache:master Apr 25, 2018

vinishjail97 pushed a commit to vinishjail97/hudi that referenced this pull request Dec 15, 2023

As queryInfo was not updated inplace, updating it in s3/gcs sources. (a…

b1bd1f2

…pache#380)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for updating duplicate records in same/different files in same pa… #380

Fix for updating duplicate records in same/different files in same pa… #380

suniluber commented Apr 16, 2018

CLAassistant commented Apr 16, 2018 •

edited

Loading

ovj commented Apr 17, 2018

vinothchandar commented Apr 17, 2018

suniluber commented Apr 17, 2018

vinothchandar Apr 17, 2018

suniluber Apr 17, 2018

vinothchandar Apr 20, 2018

vinothchandar Apr 17, 2018

vinothchandar Apr 17, 2018

vinothchandar Apr 17, 2018

vinothchandar Apr 17, 2018

vinothchandar Apr 17, 2018

vinothchandar Apr 17, 2018

bvaradar left a comment

suniluber commented Apr 24, 2018

vinothchandar commented Apr 25, 2018

vinothchandar Apr 25, 2018

vinothchandar Apr 25, 2018

suniluber Apr 25, 2018 •

edited

Loading

vinothchandar Apr 25, 2018

vinothchandar Apr 25, 2018

vinothchandar left a comment

		}

		Dataset<Row> dataSet = HoodieClientTestUtils.read(basePath, sqlContext, fs, fullPartitionPaths);

Fix for updating duplicate records in same/different files in same pa… #380

Fix for updating duplicate records in same/different files in same pa… #380

Conversation

suniluber commented Apr 16, 2018

CLAassistant commented Apr 16, 2018 • edited Loading

ovj commented Apr 17, 2018

vinothchandar commented Apr 17, 2018

suniluber commented Apr 17, 2018

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 left a comment

Choose a reason for hiding this comment

suniluber commented Apr 24, 2018

vinothchandar commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suniluber Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinothchandar left a comment

Choose a reason for hiding this comment

CLAassistant commented Apr 16, 2018 •

edited

Loading

suniluber Apr 25, 2018 •

edited

Loading