You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2020/03/11 03:08:52 UTC

[GitHub] [incubator-hudi] satishkotha opened a new pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

satishkotha opened a new pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396
 
 
   ## What is the purpose of the pull request
   example timeline:
   
   t0 -> create bucket1.parquet
   t1 -> create and append updates bucket1.log
   t2 -> request compaction
   t3 -> create bucket2.parquet
   
   if compaction at t2 takes a long time, incremental reads using HoodieParquetInputFormat may make progress to read commits at t3 and skip data ingested at t1 leading to 'data loss' .(Data will still be on disk, but incremental readers wont see it because its in log file and readers move to t3)
   
   To workaround this problem, we want to stop returning data belonging to commits > compaction_requested/inprogress_instant. After compaction is complete, incremental reader would see updates in t2, t3, so on. Disadvantage is that long running compactions can make it look like reader is 'stuck'. But that is better than skipping updates.
   
   ## Brief change log
   
   - Change HoodieParquetInputFormat to read commits prior to compaction instant
   - Added unit tests to validate behavior
   - Fix broken test utils for reading records
   
   ## Verify this pull request
   This change added tests and can be verified as follows:
   mvn test (TestMergeOnReadTable and TestHoodieActiveTimeline)
   
   Some discussion is on https://github.com/apache/incubator-hudi/pull/1389, sorry I messed up rebase, so resending as a new PR to avoid confusion
   
   ## 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.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] bvaradar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

bvaradar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r395181942
 
 

 ##########
 File path: hudi-client/src/test/java/org/apache/hudi/common/HoodieMergeOnReadTestUtils.java
 ##########
 @@ -47,27 +48,35 @@
 
   public static List<GenericRecord> getRecordsUsingInputFormat(List<String> inputPaths, String basePath) {
     JobConf jobConf = new JobConf();
+    return getRecordsUsingInputFormat(inputPaths, basePath, jobConf, new HoodieParquetRealtimeInputFormat());
+  }
+
+  public static List<GenericRecord> getRecordsUsingInputFormat(List<String> inputPaths,
+                                                               String basePath,
+                                                               JobConf jobConf,
+                                                               HoodieParquetInputFormat inputFormat) {
     Schema schema = HoodieAvroUtils.addMetadataFields(
         new Schema.Parser().parse(HoodieTestDataGenerator.TRIP_EXAMPLE_SCHEMA));
-    HoodieParquetRealtimeInputFormat inputFormat = new HoodieParquetRealtimeInputFormat();
     setPropsForInputFormat(inputFormat, jobConf, schema, basePath);
     return inputPaths.stream().map(path -> {
       setInputPath(jobConf, path);
       List<GenericRecord> records = new ArrayList<>();
       try {
         List<InputSplit> splits = Arrays.asList(inputFormat.getSplits(jobConf, 1));
-        RecordReader recordReader = inputFormat.getRecordReader(splits.get(0), jobConf, null);
-        Void key = (Void) recordReader.createKey();
-        ArrayWritable writable = (ArrayWritable) recordReader.createValue();
-        while (recordReader.next(key, writable)) {
-          GenericRecordBuilder newRecord = new GenericRecordBuilder(schema);
-          // writable returns an array with [field1, field2, _hoodie_commit_time,
-          // _hoodie_commit_seqno]
-          Writable[] values = writable.get();
-          schema.getFields().forEach(field -> {
-            newRecord.set(field, values[2]);
-          });
-          records.add(newRecord.build());
+        for (InputSplit split : splits) {
 
 Review comment:
   Was this working because these tests only dealt with 1 file-groups ? I can see count based assertions using records collected from this method 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-609849702
 
 
   @bhasudha is fixing the RT incremental pull. Hopefully this is all much easier.. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] bvaradar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

bvaradar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r395185815
 
 

 ##########
 File path: hudi-common/src/test/java/org/apache/hudi/common/table/string/TestHoodieActiveTimeline.java
 ##########
 @@ -160,6 +159,8 @@ public void testTimelineOperations() {
         .filterCompletedInstants().findInstantsInRange("04", "11").getInstants().map(HoodieInstant::getTimestamp));
     HoodieTestUtils.assertStreamEquals("", Stream.of("09", "11"), timeline.getCommitTimeline().filterCompletedInstants()
         .findInstantsAfter("07", 2).getInstants().map(HoodieInstant::getTimestamp));
+    HoodieTestUtils.assertStreamEquals("", Stream.of("01", "03", "05"), timeline.getCommitTimeline().filterCompletedInstants()
 
 Review comment:
   nit: Add non-empty message for the assertion.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io edited a comment on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-597430876
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=h1) Report
   > Merging [#1396](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/b5d093a21bbb19f164fbc549277188f2151232a8&el=desc) will **increase** coverage by `0.66%`.
   > The diff coverage is `95.65%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1396/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1396      +/-   ##
   ============================================
   + Coverage     71.53%   72.19%   +0.66%     
   - Complexity      261      290      +29     
   ============================================
     Files           336      338       +2     
     Lines         15744    15949     +205     
     Branches       1610     1627      +17     
   ============================================
   + Hits          11263    11515     +252     
   + Misses         3760     3704      -56     
   - Partials        721      730       +9     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...che/hudi/common/table/timeline/HoodieTimeline.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3RpbWVsaW5lL0hvb2RpZVRpbWVsaW5lLmphdmE=) | `100.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...oop/realtime/HoodieParquetRealtimeInputFormat.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1oYWRvb3AtbXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvaGFkb29wL3JlYWx0aW1lL0hvb2RpZVBhcnF1ZXRSZWFsdGltZUlucHV0Rm9ybWF0LmphdmE=) | `72.34% <0.00%> (-0.78%)` | `0.00 <0.00> (ø)` | |
   | [...i/common/table/timeline/HoodieDefaultTimeline.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3RpbWVsaW5lL0hvb2RpZURlZmF1bHRUaW1lbGluZS5qYXZh) | `92.42% <100.00%> (+0.23%)` | `0.00 <0.00> (ø)` | |
   | [...in/java/org/apache/hudi/hadoop/HoodieHiveUtil.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1oYWRvb3AtbXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvaGFkb29wL0hvb2RpZUhpdmVVdGlsLmphdmE=) | `87.09% <100.00%> (+1.91%)` | `0.00 <0.00> (ø)` | |
   | [...g/apache/hudi/hadoop/HoodieParquetInputFormat.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1oYWRvb3AtbXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvaGFkb29wL0hvb2RpZVBhcnF1ZXRJbnB1dEZvcm1hdC5qYXZh) | `84.32% <100.00%> (+3.33%)` | `0.00 <0.00> (ø)` | |
   | [...apache/hudi/utilities/deltastreamer/DeltaSync.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvRGVsdGFTeW5jLmphdmE=) | `71.85% <0.00%> (-0.15%)` | `38.00% <0.00%> (ø%)` | |
   | [...s/deltastreamer/HoodieMultiTableDeltaStreamer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvSG9vZGllTXVsdGlUYWJsZURlbHRhU3RyZWFtZXIuamF2YQ==) | `78.88% <0.00%> (ø)` | `18.00% <0.00%> (?%)` | |
   | [...utilities/deltastreamer/TableExecutionContext.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvVGFibGVFeGVjdXRpb25Db250ZXh0LmphdmE=) | `65.00% <0.00%> (ø)` | `9.00% <0.00%> (?%)` | |
   | [...main/scala/org/apache/hudi/DataSourceOptions.scala](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1zcGFyay9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2h1ZGkvRGF0YVNvdXJjZU9wdGlvbnMuc2NhbGE=) | `93.25% <0.00%> (+0.07%)` | `0.00% <0.00%> (ø%)` | |
   | [...i/utilities/deltastreamer/HoodieDeltaStreamer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvSG9vZGllRGVsdGFTdHJlYW1lci5qYXZh) | `78.77% <0.00%> (+0.16%)` | `10.00% <0.00%> (+2.00%)` | |
   | ... and [12 more](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=footer). Last update [b5d093a...6926049](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] bvaradar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

bvaradar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r395187395
 
 

 ##########
 File path: hudi-client/src/test/java/org/apache/hudi/table/TestMergeOnReadTable.java
 ##########
 @@ -80,6 +85,12 @@
 
 public class TestMergeOnReadTable extends HoodieClientTestHarness {
 
+  private HoodieParquetInputFormat roInputFormat;
 
 Review comment:
   We can add similar test-cases for Copy-On-Write table with RO inputformat alone to make it uniform. @satishkotha : Can you look at it as part of this PR or a follow-up ?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r403839376
 
 

 ##########
 File path: hudi-client/src/test/java/org/apache/hudi/table/TestMergeOnReadTable.java
 ##########
 @@ -186,6 +168,96 @@ public void testSimpleInsertAndUpdate() throws Exception {
     }
   }
 
+  // test incremental read does not go past compaction instant for RO views
+  // For RT views, incremental read can go past compaction
+  @Test
+  public void testIncrementalReadsWithCompaction() throws Exception {
 
 Review comment:
   This actually includes RT views too.
   
   Example line 195: getRTIncrementalFiles(partitionPath); validateIncrementalFiles()

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-598409536
 
 
   > > if compaction at t2 takes a long time, incremental reads using HoodieParquetInputFormat may make progress to read commits at t3
   > 
   > IIUC this is because you are incremental pulling from the parquet only table? I thought we can already incremental pull via logs. no? cc @n3nash .. is this really needed since it will add complexity to the system..
   > 
   > Eventually, I would like incremental query/pull on MOR to be just based on logs..
   
   Based on view type, hudi decides the input format to use (see https://github.com/apache/incubator-hudi/blob/master/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncTool.java#L91) . For RO views, we use HoodieParquetInputFormat, which does not read log files. For RT views, we use HoodieParquetRealtimeInputFormat, which reads slice including log files. In my limited testing, incremental reads on RT views also do not work well (we see duplicates after compaction in some conditions).  @bvaradar  is working on fixing any broken windows for supporting incremental reads on RT views.
   
   We wanted to include this change for supporting RO views (which is majority of use cases for us). I agree with you that this is additional complexity. I added more tests than usual because of that. 
   
   Other alternatives i can think of:
   1) Support incremental reads only for RT views.  incremental reads on RO can fail or use RT (is this your proposal in the above comment?)
   2) Instead of doing incremental reads based on hoodie commit time, use parquet file creation times. This approach requires substantial changes and likely be breaking some fundamental assumptions.
   
   Also, at a high level, I want to discuss adding additional mode for incremental reads. Today, its responsibility of hoodie users to save commit times and use that for next incremental reads. Can we add 'kafka consumer' model, where consumer only specifies their unique-id. Hudi tracks read progress (perhaps as part of consolidated metadata?). This would simplify usage and make debugging lot easier.
    
   fyi,  @n3nash is out of office for next 10 days. @bvaradar likely can share more context. Let me know if you have other suggestions.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-598336174
 
 
   > if compaction at t2 takes a long time, incremental reads using HoodieParquetInputFormat may make progress to read commits at t3
   
   IIUC this is because you are incremental pulling from the parquet only table? I thought we can already incremental pull via logs. no? cc @n3nash .. is this really needed since it will add complexity to the system..
   
   Eventually, I would like incremental query/pull on MOR to be just based on logs.. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r403836768
 
 

 ##########
 File path: hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java
 ##########
 @@ -118,6 +119,34 @@
     return returns.toArray(new FileStatus[returns.size()]);
   }
 
+  /**
+   * Filter any specific instants that we do not want to process.
+   * example timeline:
+   *
+   * t0 -> create bucket1.parquet
+   * t1 -> create and append updates bucket1.log
+   * t2 -> request compaction
+   * t3 -> create bucket2.parquet
+   *
+   * if compaction at t2 takes a long time, incremental readers on RO tables can move to t3 and would skip updates in t1
+   *
+   * To workaround this problem, we want to stop returning data belonging to commits > t2.
+   * After compaction is complete, incremental reader would see updates in t2, t3, so on.
+   */
+  protected HoodieDefaultTimeline filterInstantsTimeline(HoodieDefaultTimeline timeline) {
+    Option<HoodieInstant> pendingCompactionInstant = timeline.filterPendingCompactionTimeline().firstInstant();
+    if (pendingCompactionInstant.isPresent()) {
 
 Review comment:
   Yes, this is the crux of the change. My understanding is this bug caused data loss on derived ETL tables multiple times. These ETL tables are generated using incremental reads on "RO"views. As you suggested, that is core issue and switching to RT views is likely going to get rid of the problem. 
   
   Also given "getting started" and other demo examples  include incremental reads on RO views,  I think this is useful to have especially given that finding root cause  for this took a while.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-611202945
 
 
   @bvaradar Please take a final pass and merge.. I don't have concerns to move forward.. cc @satishkotha 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r392043404
 
 

 ##########
 File path: hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java
 ##########
 @@ -116,6 +117,24 @@
     return returns.toArray(new FileStatus[returns.size()]);
   }
 
+  /**
+   * Filter any specific instants that we do not want to process.
+   * example timeline:
+   *
+   * t0 -> create bucket1.parquet
+   * t1 -> create and append updates bucket1.log
+   * t2 -> request compaction
+   * t3 -> create bucket2.parquet
+   *
+   * if compaction at t2 takes a long time, incremental readers on RO tables can move to t3 and would skip updates in t1
+   *
+   * To workaround this problem, we want to stop returning data belonging to commits > t2.
+   * After compaction is complete, incremental reader would see updates in t2, t3, so on.
+   */
+  protected HoodieTimeline filterInstantsTimeline(HoodieTimeline timeline) {
 
 Review comment:
   lets move the code from `HoodieDefaultTimeline` here? 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r395386191
 
 

 ##########
 File path: hudi-client/src/test/java/org/apache/hudi/common/HoodieMergeOnReadTestUtils.java
 ##########
 @@ -47,27 +48,35 @@
 
   public static List<GenericRecord> getRecordsUsingInputFormat(List<String> inputPaths, String basePath) {
     JobConf jobConf = new JobConf();
+    return getRecordsUsingInputFormat(inputPaths, basePath, jobConf, new HoodieParquetRealtimeInputFormat());
+  }
+
+  public static List<GenericRecord> getRecordsUsingInputFormat(List<String> inputPaths,
+                                                               String basePath,
+                                                               JobConf jobConf,
+                                                               HoodieParquetInputFormat inputFormat) {
     Schema schema = HoodieAvroUtils.addMetadataFields(
         new Schema.Parser().parse(HoodieTestDataGenerator.TRIP_EXAMPLE_SCHEMA));
-    HoodieParquetRealtimeInputFormat inputFormat = new HoodieParquetRealtimeInputFormat();
     setPropsForInputFormat(inputFormat, jobConf, schema, basePath);
     return inputPaths.stream().map(path -> {
       setInputPath(jobConf, path);
       List<GenericRecord> records = new ArrayList<>();
       try {
         List<InputSplit> splits = Arrays.asList(inputFormat.getSplits(jobConf, 1));
-        RecordReader recordReader = inputFormat.getRecordReader(splits.get(0), jobConf, null);
-        Void key = (Void) recordReader.createKey();
-        ArrayWritable writable = (ArrayWritable) recordReader.createValue();
-        while (recordReader.next(key, writable)) {
-          GenericRecordBuilder newRecord = new GenericRecordBuilder(schema);
-          // writable returns an array with [field1, field2, _hoodie_commit_time,
-          // _hoodie_commit_seqno]
-          Writable[] values = writable.get();
-          schema.getFields().forEach(field -> {
-            newRecord.set(field, values[2]);
-          });
-          records.add(newRecord.build());
+        for (InputSplit split : splits) {
 
 Review comment:
   yes, correct.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] bvaradar merged pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

bvaradar merged pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r392436908
 
 

 ##########
 File path: hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java
 ##########
 @@ -116,6 +117,24 @@
     return returns.toArray(new FileStatus[returns.size()]);
   }
 
+  /**
+   * Filter any specific instants that we do not want to process.
+   * example timeline:
+   *
+   * t0 -> create bucket1.parquet
+   * t1 -> create and append updates bucket1.log
+   * t2 -> request compaction
+   * t3 -> create bucket2.parquet
+   *
+   * if compaction at t2 takes a long time, incremental readers on RO tables can move to t3 and would skip updates in t1
+   *
+   * To workaround this problem, we want to stop returning data belonging to commits > t2.
+   * After compaction is complete, incremental reader would see updates in t2, t3, so on.
+   */
+  protected HoodieTimeline filterInstantsTimeline(HoodieTimeline timeline) {
 
 Review comment:
   done

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io edited a comment on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

codecov-io edited a comment on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-597430876
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=h1) Report
   > Merging [#1396](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/b5d093a21bbb19f164fbc549277188f2151232a8&el=desc) will **increase** coverage by `0.66%`.
   > The diff coverage is `95.65%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1396/graphs/tree.svg?width=650&height=150&src=pr&token=VTTXabwbs2)](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1396      +/-   ##
   ============================================
   + Coverage     71.53%   72.19%   +0.66%     
   - Complexity      261      290      +29     
   ============================================
     Files           336      338       +2     
     Lines         15744    15949     +205     
     Branches       1610     1627      +17     
   ============================================
   + Hits          11263    11515     +252     
   + Misses         3760     3704      -56     
   - Partials        721      730       +9     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...che/hudi/common/table/timeline/HoodieTimeline.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3RpbWVsaW5lL0hvb2RpZVRpbWVsaW5lLmphdmE=) | `100.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |
   | [...oop/realtime/HoodieParquetRealtimeInputFormat.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1oYWRvb3AtbXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvaGFkb29wL3JlYWx0aW1lL0hvb2RpZVBhcnF1ZXRSZWFsdGltZUlucHV0Rm9ybWF0LmphdmE=) | `72.34% <0.00%> (-0.78%)` | `0.00 <0.00> (ø)` | |
   | [...i/common/table/timeline/HoodieDefaultTimeline.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3RpbWVsaW5lL0hvb2RpZURlZmF1bHRUaW1lbGluZS5qYXZh) | `92.42% <100.00%> (+0.23%)` | `0.00 <0.00> (ø)` | |
   | [...in/java/org/apache/hudi/hadoop/HoodieHiveUtil.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1oYWRvb3AtbXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvaGFkb29wL0hvb2RpZUhpdmVVdGlsLmphdmE=) | `87.09% <100.00%> (+1.91%)` | `0.00 <0.00> (ø)` | |
   | [...g/apache/hudi/hadoop/HoodieParquetInputFormat.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1oYWRvb3AtbXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvaGFkb29wL0hvb2RpZVBhcnF1ZXRJbnB1dEZvcm1hdC5qYXZh) | `84.32% <100.00%> (+3.33%)` | `0.00 <0.00> (ø)` | |
   | [...apache/hudi/utilities/deltastreamer/DeltaSync.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvRGVsdGFTeW5jLmphdmE=) | `71.85% <0.00%> (-0.15%)` | `38.00% <0.00%> (ø%)` | |
   | [...utilities/deltastreamer/TableExecutionContext.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvVGFibGVFeGVjdXRpb25Db250ZXh0LmphdmE=) | `65.00% <0.00%> (ø)` | `9.00% <0.00%> (?%)` | |
   | [...s/deltastreamer/HoodieMultiTableDeltaStreamer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvSG9vZGllTXVsdGlUYWJsZURlbHRhU3RyZWFtZXIuamF2YQ==) | `78.88% <0.00%> (ø)` | `18.00% <0.00%> (?%)` | |
   | [...main/scala/org/apache/hudi/DataSourceOptions.scala](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1zcGFyay9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2h1ZGkvRGF0YVNvdXJjZU9wdGlvbnMuc2NhbGE=) | `93.25% <0.00%> (+0.07%)` | `0.00% <0.00%> (ø%)` | |
   | [...i/utilities/deltastreamer/HoodieDeltaStreamer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvSG9vZGllRGVsdGFTdHJlYW1lci5qYXZh) | `78.77% <0.00%> (+0.16%)` | `10.00% <0.00%> (+2.00%)` | |
   | ... and [12 more](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=footer). Last update [b5d093a...6926049](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha edited a comment on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha edited a comment on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-606157488
 
 
   > Looks great @satishkotha Once you rebase and fix conflicts, we should be good to commit.
   
   @vinothchandar Checking to see if you have any other feedback here as you added few comments?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r392436855
 
 

 ##########
 File path: hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTimeline.java
 ##########
 @@ -127,6 +127,24 @@
    */
   HoodieTimeline getCommitsAndCompactionTimeline();
 
+
+  /**
+   * Get all instants (commits, delta commits) that produce new data, in the active timeline.
+   */
+  HoodieTimeline getCommitsTimeline();
 
 Review comment:
   I removed it 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r392042671
 
 

 ##########
 File path: hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTimeline.java
 ##########
 @@ -144,6 +162,11 @@
    */
   HoodieTimeline findInstantsAfter(String commitTime, int numCommits);
 
+  /**
+   * Create a new Timeline with all instants before specified time.
+   */
+  HoodieTimeline findInstantsBefore(String time);
 
 Review comment:
   rename: instantTime

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r404162013

##########
File path: hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java
##########
@@ -118,6 +119,34 @@
return returns.toArray(new FileStatus[returns.size()]);
}

+ /**
+ * Filter any specific instants that we do not want to process.
+ * example timeline:
+ *
+ * t0 -> create bucket1.parquet
+ * t1 -> create and append updates bucket1.log
+ * t2 -> request compaction
+ * t3 -> create bucket2.parquet
+ *
+ * if compaction at t2 takes a long time, incremental readers on RO tables can move to t3 and would skip updates in t1
+ *
+ * To workaround this problem, we want to stop returning data belonging to commits > t2.
+ * After compaction is complete, incremental reader would see updates in t2, t3, so on.
+ */
+ protected HoodieDefaultTimeline filterInstantsTimeline(HoodieDefaultTimeline timeline) {
+ Option<HoodieInstant> pendingCompactionInstant = timeline.filterPendingCompactionTimeline().firstInstant();
+ if (pendingCompactionInstant.isPresent()) {

Review comment:
Once again this is not a bug :) .. Its not supposed to be used this way.. I warned against this in fact, in the past as well .. anyways, water under the bridge..

I can see how this approach specifically helps the way your datasets... I am fine landing this change per se.. Let's introduce a jobConf variable to control this behavior? we can turn this off by default and you can ask the derived ETL to turn this on? (I am find making it default to on also, your call)

To be on the same page, thinking out loud.. This change is orthogonal to the compaction strategy right.. for e.g , the compaction may not compact all the data in log (lets say it only compacts the last N partitions), then the records from older partitions won't show in the change stream right until later... But what this fixes is avoiding incremental reader to advance ahead when it sees a compaction... (The data skipping happens because compaction happens at a much later time and incremental reader moves onto other delta commits for e.g)..

P.S: Another things to double check is that the records written into base file during compaction, have the `_hoodie_commit_time` of the compaction and not the original write.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r403838755
 
 

 ##########
 File path: hudi-client/src/test/java/org/apache/hudi/table/TestMergeOnReadTable.java
 ##########
 @@ -1311,4 +1383,111 @@ private void assertNoWriteErrors(List<WriteStatus> statuses) {
       assertFalse("Errors found in write of " + status.getFileId(), status.hasErrors());
     }
   }
+  
+  private FileStatus[] insertAndGetFilePaths(List<HoodieRecord> records, HoodieWriteClient client,
 
 Review comment:
   This helper code is refactored from existing test 'TestMergeOnReadTable#testSimpleInsertAndUpdate' . I don't know this setup is needed in other places. If you have any suggestions for new location, let me know.
   
   There are also no tests so far with end-to-end testing of incremental reads on MOR tables (as far as i could tell). So I had to add some more utility test methods for that. Please suggest if ythere is a better location for that

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-603902158
 
 
   if @bvaradar approves, it should be sufficient.. I will take another pass at it today

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r395386006
 
 

 ##########
 File path: hudi-client/src/test/java/org/apache/hudi/table/TestMergeOnReadTable.java
 ##########
 @@ -80,6 +85,12 @@
 
 public class TestMergeOnReadTable extends HoodieClientTestHarness {
 
+  private HoodieParquetInputFormat roInputFormat;
 
 Review comment:
   Thanks for suggestion. I modified one of the existing tests on COW table. Please take a look. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r392436630
 
 

 ##########
 File path: hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTimeline.java
 ##########
 @@ -127,6 +127,24 @@
    */
   HoodieTimeline getCommitsAndCompactionTimeline();
 
+
+  /**
+   * Get all instants (commits, delta commits) that produce new data, in the active timeline.
+   */
+  HoodieTimeline getCommitsTimeline();
+
+  /**
+   * Timeline to include all instants before the first 'requested compaction'.
+   *
+   * For example, say timeline: commit at t0, deltacommit at t1, compaction requested at t2, deltacommit at t3,
+   * this method would return t0, t1
+   *
+   * If there is no pending compaction this is equivalent to getting all instants in the timeline.
+   *
+   * @return
+   */
+  HoodieTimeline filterInstantsBeforePendingCompaction();
 
 Review comment:
   Moved it to HoodieParquetInputFormat as suggested

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r404159491
 
 

 ##########
 File path: hudi-client/src/test/java/org/apache/hudi/table/TestMergeOnReadTable.java
 ##########
 @@ -1311,4 +1383,111 @@ private void assertNoWriteErrors(List<WriteStatus> statuses) {
       assertFalse("Errors found in write of " + status.getFileId(), status.hasErrors());
     }
   }
+  
+  private FileStatus[] insertAndGetFilePaths(List<HoodieRecord> records, HoodieWriteClient client,
 
 Review comment:
   let's have it like this for now.. Thanks for the explanation..! 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r403836768
 
 

 ##########
 File path: hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java
 ##########
 @@ -118,6 +119,34 @@
     return returns.toArray(new FileStatus[returns.size()]);
   }
 
+  /**
+   * Filter any specific instants that we do not want to process.
+   * example timeline:
+   *
+   * t0 -> create bucket1.parquet
+   * t1 -> create and append updates bucket1.log
+   * t2 -> request compaction
+   * t3 -> create bucket2.parquet
+   *
+   * if compaction at t2 takes a long time, incremental readers on RO tables can move to t3 and would skip updates in t1
+   *
+   * To workaround this problem, we want to stop returning data belonging to commits > t2.
+   * After compaction is complete, incremental reader would see updates in t2, t3, so on.
+   */
+  protected HoodieDefaultTimeline filterInstantsTimeline(HoodieDefaultTimeline timeline) {
+    Option<HoodieInstant> pendingCompactionInstant = timeline.filterPendingCompactionTimeline().firstInstant();
+    if (pendingCompactionInstant.isPresent()) {
 
 Review comment:
   Yes, this is the crux of the change. My understanding is this bug caused data loss on derived ETL tables multiple times. These ETL tables are generated using incremental reads on "RO"views. As you suggested, that is core issue and switching to RT views is likely going to get rid of the problem. 
   
   Also given "getting started" and other demo examples  include incremental reads on RO views,  I think this new safeguard is useful to have, especially given that finding root cause  for this took a while.  I am fine with abandoning this change if we can remove incremental read examples on RO views in documentation.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r395386043
 
 

 ##########
 File path: hudi-common/src/test/java/org/apache/hudi/common/table/string/TestHoodieActiveTimeline.java
 ##########
 @@ -160,6 +159,8 @@ public void testTimelineOperations() {
         .filterCompletedInstants().findInstantsInRange("04", "11").getInstants().map(HoodieInstant::getTimestamp));
     HoodieTestUtils.assertStreamEquals("", Stream.of("09", "11"), timeline.getCommitTimeline().filterCompletedInstants()
         .findInstantsAfter("07", 2).getInstants().map(HoodieInstant::getTimestamp));
+    HoodieTestUtils.assertStreamEquals("", Stream.of("01", "03", "05"), timeline.getCommitTimeline().filterCompletedInstants()
 
 Review comment:
   fixed

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-606157488
 
 
   > Looks great @satishkotha Once you rebase and fix conflicts, we should be good to commit.
   
   @vinothchandar Checking to see if you have any other feedback here?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r392043150
 
 

 ##########
 File path: hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTimeline.java
 ##########
 @@ -127,6 +127,24 @@
    */
   HoodieTimeline getCommitsAndCompactionTimeline();
 
+
+  /**
+   * Get all instants (commits, delta commits) that produce new data, in the active timeline.
+   */
+  HoodieTimeline getCommitsTimeline();
+
+  /**
+   * Timeline to include all instants before the first 'requested compaction'.
+   *
+   * For example, say timeline: commit at t0, deltacommit at t1, compaction requested at t2, deltacommit at t3,
+   * this method would return t0, t1
+   *
+   * If there is no pending compaction this is equivalent to getting all instants in the timeline.
+   *
+   * @return
+   */
+  HoodieTimeline filterInstantsBeforePendingCompaction();
 
 Review comment:
   In any case, rename to `filterInstantsBeforeFirstPendingCompaction`? 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r392043054
 
 

 ##########
 File path: hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTimeline.java
 ##########
 @@ -127,6 +127,24 @@
    */
   HoodieTimeline getCommitsAndCompactionTimeline();
 
+
+  /**
+   * Get all instants (commits, delta commits) that produce new data, in the active timeline.
+   */
+  HoodieTimeline getCommitsTimeline();
+
+  /**
+   * Timeline to include all instants before the first 'requested compaction'.
+   *
+   * For example, say timeline: commit at t0, deltacommit at t1, compaction requested at t2, deltacommit at t3,
+   * this method would return t0, t1
+   *
+   * If there is no pending compaction this is equivalent to getting all instants in the timeline.
+   *
+   * @return
+   */
+  HoodieTimeline filterInstantsBeforePendingCompaction();
 
 Review comment:
   Should this belong in the HoodieTimeline class itself? It seems like you can easily build this using other methods, outside of HoodieTimeline? 
   
   Given this is an interim fix, I would like this to be as isolated as possible from the core classes.. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] bvaradar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

bvaradar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r399958899
 
 

 ##########
 File path: hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieDefaultTimeline.java
 ##########
 @@ -136,6 +136,13 @@ public HoodieDefaultTimeline findInstantsAfter(String instantTime, int numCommit
         details);
   }
 
+  @Override
+  public HoodieDefaultTimeline findInstantsBefore(String instantTime) {
 
 Review comment:
   Didn't realize you can have an overridden method with a return type which is a sub-type of original return type. Learnt something new today :) 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-601473026
 
 
   > @satishkotha : Some minor comments. Will approve once you reply/address them. Let's also wait for @vinothchandar to take a pass
   
   @bvaradar Sounds good. I addressed your review comments.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r403838755
 
 

 ##########
 File path: hudi-client/src/test/java/org/apache/hudi/table/TestMergeOnReadTable.java
 ##########
 @@ -1311,4 +1383,111 @@ private void assertNoWriteErrors(List<WriteStatus> statuses) {
       assertFalse("Errors found in write of " + status.getFileId(), status.hasErrors());
     }
   }
+  
+  private FileStatus[] insertAndGetFilePaths(List<HoodieRecord> records, HoodieWriteClient client,
 
 Review comment:
   This helper code is refactored from existing test 'TestMergeOnReadTable#testSimpleInsertAndUpdate' . I don't know this setup is needed in other places. If you have any suggestions for new location, let me know.
   
   There are also no tests so far with end-to-end testing of incremental reads on MOR tables (as far as i could tell). So I had to add some more utility test methods for that. Please suggest if there is a better location for that

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-602769754
 
 
   @vinothchandar could you take another look at this one?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r403397923
 
 

 ##########
 File path: hudi-client/src/test/java/org/apache/hudi/table/TestMergeOnReadTable.java
 ##########
 @@ -186,6 +168,96 @@ public void testSimpleInsertAndUpdate() throws Exception {
     }
   }
 
+  // test incremental read does not go past compaction instant for RO views
+  // For RT views, incremental read can go past compaction
+  @Test
+  public void testIncrementalReadsWithCompaction() throws Exception {
 
 Review comment:
   Let's make it clear that this an incremental on RO.. rename to `testIncrementalROReadsWithCOmpaction()`? 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-598563329
 
 
   >>(is this your proposal in the above comment?)
   
   yes.. Breaking away from _hoodie_commit_time is not a good approach, due to the reasons you cited. I agree there.. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r403838755
 
 

 ##########
 File path: hudi-client/src/test/java/org/apache/hudi/table/TestMergeOnReadTable.java
 ##########
 @@ -1311,4 +1383,111 @@ private void assertNoWriteErrors(List<WriteStatus> statuses) {
       assertFalse("Errors found in write of " + status.getFileId(), status.hasErrors());
     }
   }
+  
+  private FileStatus[] insertAndGetFilePaths(List<HoodieRecord> records, HoodieWriteClient client,
 
 Review comment:
   This one is refactored from existing test 'TestMergeOnReadTable#testSimpleInsertAndUpdate' . I don't know this setup is needed in other places. If you have any suggestions for new location, let me know.
   
   There are also no tests so far with end-to-end testing of incremental reads on MOR tables (as far as i could tell). So I had to add some more utility test methods for that. Please suggest if ythere is a better location for that

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] codecov-io commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

codecov-io commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-597430876
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=h1) Report
   > Merging [#1396](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-hudi/commit/77d5b92d88d6583bdfc09e4c10ecfe7ddbb04806?src=pr&el=desc) will **increase** coverage by `<.01%`.
   > The diff coverage is `100%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-hudi/pull/1396/graphs/tree.svg?width=650&token=VTTXabwbs2&height=150&src=pr)](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##             master   #1396      +/-   ##
   ===========================================
   + Coverage      67.4%   67.4%   +<.01%     
     Complexity      230     230              
   ===========================================
     Files           336     336              
     Lines         16366   16379      +13     
     Branches       1672    1673       +1     
   ===========================================
   + Hits          11031   11041      +10     
   - Misses         4602    4603       +1     
   - Partials        733     735       +2
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...a/org/apache/hudi/common/table/HoodieTimeline.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL0hvb2RpZVRpbWVsaW5lLmphdmE=) | `100% <ø> (ø)` | `0 <0> (ø)` | :arrow_down: |
   | [...oop/realtime/HoodieParquetRealtimeInputFormat.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1oYWRvb3AtbXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvaGFkb29wL3JlYWx0aW1lL0hvb2RpZVBhcnF1ZXRSZWFsdGltZUlucHV0Rm9ybWF0LmphdmE=) | `73.4% <100%> (+0.28%)` | `0 <0> (ø)` | :arrow_down: |
   | [...i/common/table/timeline/HoodieDefaultTimeline.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL3RhYmxlL3RpbWVsaW5lL0hvb2RpZURlZmF1bHRUaW1lbGluZS5qYXZh) | `93.24% <100%> (+1.05%)` | `0 <0> (ø)` | :arrow_down: |
   | [...g/apache/hudi/hadoop/HoodieParquetInputFormat.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1oYWRvb3AtbXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvaGFkb29wL0hvb2RpZVBhcnF1ZXRJbnB1dEZvcm1hdC5qYXZh) | `81.3% <100%> (+1.96%)` | `0 <0> (ø)` | :arrow_down: |
   | [...g/apache/hudi/metrics/InMemoryMetricsReporter.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1jbGllbnQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvbWV0cmljcy9Jbk1lbW9yeU1ldHJpY3NSZXBvcnRlci5qYXZh) | `25% <0%> (-50%)` | `0% <0%> (ø)` | |
   | [...n/java/org/apache/hudi/common/model/HoodieKey.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvY29tbW9uL21vZGVsL0hvb2RpZUtleS5qYXZh) | `88.88% <0%> (-5.56%)` | `0% <0%> (ø)` | |
   | [...i/utilities/deltastreamer/HoodieDeltaStreamer.java](https://codecov.io/gh/apache/incubator-hudi/pull/1396/diff?src=pr&el=tree#diff-aHVkaS11dGlsaXRpZXMvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2h1ZGkvdXRpbGl0aWVzL2RlbHRhc3RyZWFtZXIvSG9vZGllRGVsdGFTdHJlYW1lci5qYXZh) | `79.79% <0%> (-1.02%)` | `8% <0%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=footer). Last update [77d5b92...a023c31](https://codecov.io/gh/apache/incubator-hudi/pull/1396?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r403397635
 
 

 ##########
 File path: hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java
 ##########
 @@ -118,6 +119,34 @@
     return returns.toArray(new FileStatus[returns.size()]);
   }
 
+  /**
+   * Filter any specific instants that we do not want to process.
+   * example timeline:
+   *
+   * t0 -> create bucket1.parquet
+   * t1 -> create and append updates bucket1.log
+   * t2 -> request compaction
+   * t3 -> create bucket2.parquet
+   *
+   * if compaction at t2 takes a long time, incremental readers on RO tables can move to t3 and would skip updates in t1
+   *
+   * To workaround this problem, we want to stop returning data belonging to commits > t2.
+   * After compaction is complete, incremental reader would see updates in t2, t3, so on.
+   */
+  protected HoodieDefaultTimeline filterInstantsTimeline(HoodieDefaultTimeline timeline) {
+    Option<HoodieInstant> pendingCompactionInstant = timeline.filterPendingCompactionTimeline().firstInstant();
+    if (pendingCompactionInstant.isPresent()) {
 
 Review comment:
   This seems like the crux of the change? and most of the other code is improving tests etc. If so, this seems like a  reasonable interim solution to me... Although we should encourage users to do incremental pull out of the RTInputFormat really ... 
   
   The core problem of "data loss" being brought in this issue, feels like a mis-expectation really :) 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r403398166
 
 

 ##########
 File path: hudi-client/src/test/java/org/apache/hudi/table/TestMergeOnReadTable.java
 ##########
 @@ -1311,4 +1383,111 @@ private void assertNoWriteErrors(List<WriteStatus> statuses) {
       assertFalse("Errors found in write of " + status.getFileId(), status.hasErrors());
     }
   }
+  
+  private FileStatus[] insertAndGetFilePaths(List<HoodieRecord> records, HoodieWriteClient client,
 
 Review comment:
   I am surprised by the amount of test helper code you needed to write yourself.. Should we be using code from some existing helpers.. or should these helpers move somewhere else, so they are beneficial over all? 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r392043240
 
 

 ##########
 File path: hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTimeline.java
 ##########
 @@ -127,6 +127,24 @@
    */
   HoodieTimeline getCommitsAndCompactionTimeline();
 
+
+  /**
+   * Get all instants (commits, delta commits) that produce new data, in the active timeline.
+   */
+  HoodieTimeline getCommitsTimeline();
 
 Review comment:
   getCommitsAndCompactionTimeline wont do? 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

vinothchandar commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r405824062
 
 

 ##########
 File path: hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java
 ##########
 @@ -118,6 +119,34 @@
     return returns.toArray(new FileStatus[returns.size()]);
   }
 
+  /**
+   * Filter any specific instants that we do not want to process.
+   * example timeline:
+   *
+   * t0 -> create bucket1.parquet
+   * t1 -> create and append updates bucket1.log
+   * t2 -> request compaction
+   * t3 -> create bucket2.parquet
+   *
+   * if compaction at t2 takes a long time, incremental readers on RO tables can move to t3 and would skip updates in t1
+   *
+   * To workaround this problem, we want to stop returning data belonging to commits > t2.
+   * After compaction is complete, incremental reader would see updates in t2, t3, so on.
+   */
+  protected HoodieDefaultTimeline filterInstantsTimeline(HoodieDefaultTimeline timeline) {
+    Option<HoodieInstant> pendingCompactionInstant = timeline.filterPendingCompactionTimeline().firstInstant();
+    if (pendingCompactionInstant.isPresent()) {
 
 Review comment:
   Good point.. I think we can fix it forward since MOR incr pull issue is being fixed currently and next release, we can move the demo docs to work off _rt table.. 
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-597421941
 
 
   @bvaradar sorry, I messed up rebase on https://github.com/apache/incubator-hudi/pull/1389, Please take a look at this instead. As discussed in the other PR, I updated RO and RT views. Spark DataSource does not seem to support MOR tables, so i'm skipping that part for now. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r405144082

##########
File path: hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java
##########
@@ -118,6 +119,34 @@
return returns.toArray(new FileStatus[returns.size()]);
}

Review comment:
I can introduce jobConf variable. I'm also agreeing with you that RT is the right approach. I'm just suggesting that we remove incremental read examples in different documents or throw explicit error when someone tries incremental reads on RO views. For example, docker demo shows[ incremental reads on RO views](https://hudi.apache.org/docs/docker_demo.html#step-9-run-hive-queries-including-incremental-queries). So, people are likely to use this and end up with this difficult to debug problem.

Also, you are right about last statement. I already have tests to show that compaction timestamp is used for all updated records and not the update timestamp. Please see line 256 (last line in TestMergeOnReadTable#testIncrementalReadsWithCompaction) that does not include updateTime. We validate that records include compactionCommitTime instead

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r392436461
 
 

 ##########
 File path: hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTimeline.java
 ##########
 @@ -144,6 +162,11 @@
    */
   HoodieTimeline findInstantsAfter(String commitTime, int numCommits);
 
+  /**
+   * Create a new Timeline with all instants before specified time.
+   */
+  HoodieTimeline findInstantsBefore(String time);
 
 Review comment:
   done

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha edited a comment on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha edited a comment on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-598409536
 
 
   > > if compaction at t2 takes a long time, incremental reads using HoodieParquetInputFormat may make progress to read commits at t3
   > 
   > IIUC this is because you are incremental pulling from the parquet only table? I thought we can already incremental pull via logs. no? cc @n3nash .. is this really needed since it will add complexity to the system..
   > 
   > Eventually, I would like incremental query/pull on MOR to be just based on logs..
   
   Based on view type, hudi decides the input format to use (see https://github.com/apache/incubator-hudi/blob/master/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncTool.java#L91 and line 143) . For RO views, we use HoodieParquetInputFormat, which does not read log files. For RT views, we use HoodieParquetRealtimeInputFormat, which reads slice including log files. In my limited testing, incremental reads on RT views also do not work well (we see duplicates after compaction in some conditions).  @bvaradar  is working on fixing any broken windows for supporting incremental reads on RT views.
   
   We wanted to include this change for supporting RO views (which is majority of use cases for us). I agree with you that this is additional complexity. I added more tests than usual because of that. 
   
   Other alternatives i can think of:
   1) Support incremental reads only for RT views.  incremental reads on RO can fail or use RT (is this your proposal in the above comment?)
   2) Instead of doing incremental reads based on hoodie commit time, use parquet file creation times. This approach requires substantial changes and likely be breaking some fundamental assumptions.
   
   Also, at a high level, I want to discuss adding additional mode for incremental reads. Today, its responsibility of hoodie users to save commit times and use that for next incremental reads. Can we add 'kafka consumer' model, where consumer only specifies their unique-id. Hudi tracks read progress (perhaps as part of consolidated metadata?). This would simplify usage and make debugging lot easier.
    
   fyi,  @n3nash is out of office for next 10 days. @bvaradar likely can share more context. Let me know if you have other suggestions.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha edited a comment on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha edited a comment on issue #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#issuecomment-598409536
 
 
   > > if compaction at t2 takes a long time, incremental reads using HoodieParquetInputFormat may make progress to read commits at t3
   > 
   > IIUC this is because you are incremental pulling from the parquet only table? I thought we can already incremental pull via logs. no? cc @n3nash .. is this really needed since it will add complexity to the system..
   > 
   > Eventually, I would like incremental query/pull on MOR to be just based on logs..
   
   Based on view type, hudi decides the input format to use (see https://github.com/apache/incubator-hudi/blob/master/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncTool.java#L91 and line 143) . For RO views, we use HoodieParquetInputFormat, which does not read log files. For RT views, we use HoodieParquetRealtimeInputFormat, which reads slice including log files. @bvaradar  is working on fixing any broken windows for supporting incremental reads on RT views.
   
   We wanted to include this change for supporting RO views (which is majority of use cases for us). I agree with you that this is additional complexity. I added more tests than usual because of that. 
   
   Other alternatives i can think of:
   1) Support incremental reads only for RT views.  incremental reads on RO can fail or use RT (is this your proposal in the above comment?)
   2) Instead of doing incremental reads based on hoodie commit time, use parquet file creation times. This approach requires substantial changes and likely be breaking some fundamental assumptions.
   
   Also, at a high level, I want to discuss adding additional mode for incremental reads. Today, its responsibility of hoodie users to save commit times and use that for next incremental reads. Can we add 'kafka consumer' model, where consumer only specifies their unique-id. Hudi tracks read progress (perhaps as part of consolidated metadata?). This would simplify usage and make debugging lot easier.
    
   fyi,  @n3nash is out of office for next 10 days. @bvaradar likely can share more context. Let me know if you have other suggestions.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-hudi] satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction

Posted by GitBox <gi...@apache.org>.

satishkotha commented on a change in pull request #1396: [HUDI-687] Stop incremental reader on RO table before a pending compaction
URL: https://github.com/apache/incubator-hudi/pull/1396#discussion_r405144082
 
 

 ##########
 File path: hudi-hadoop-mr/src/main/java/org/apache/hudi/hadoop/HoodieParquetInputFormat.java
 ##########
 @@ -118,6 +119,34 @@
     return returns.toArray(new FileStatus[returns.size()]);
   }
 
+  /**
+   * Filter any specific instants that we do not want to process.
+   * example timeline:
+   *
+   * t0 -> create bucket1.parquet
+   * t1 -> create and append updates bucket1.log
+   * t2 -> request compaction
+   * t3 -> create bucket2.parquet
+   *
+   * if compaction at t2 takes a long time, incremental readers on RO tables can move to t3 and would skip updates in t1
+   *
+   * To workaround this problem, we want to stop returning data belonging to commits > t2.
+   * After compaction is complete, incremental reader would see updates in t2, t3, so on.
+   */
+  protected HoodieDefaultTimeline filterInstantsTimeline(HoodieDefaultTimeline timeline) {
+    Option<HoodieInstant> pendingCompactionInstant = timeline.filterPendingCompactionTimeline().firstInstant();
+    if (pendingCompactionInstant.isPresent()) {
 
 Review comment:
   I can introduce jobConf variable. I'm also agreeing with you that RT is the right approach. I'm just suggesting that we remove incremental read examples in different documents. For example, docker demo shows[ incremental reads on RO views](https://hudi.apache.org/docs/docker_demo.html#step-9-run-hive-queries-including-incremental-queries). So, people are likely to use this and end up with this difficult to debug problem.
   
   Also, you are right about last statement. I already have tests to show that compaction timestamp is used for all updated records and not the update timestamp. Please see line 256 (last line in TestMergeOnReadTable#testIncrementalReadsWithCompaction)  that does not include updateTime. We validate that records include compactionCommitTime instead

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services