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/08/13 23:17:05 UTC

[GitHub] [hudi] rmpifer opened a new pull request #1963: [HUDI-1188] Hbase index MOR tables records not being deduplicated

rmpifer opened a new pull request #1963:
URL: https://github.com/apache/hudi/pull/1963


   ## What is the purpose of the pull request
   
   After fetching hbase index for a record, Hudi performs validation that the commit timestamp stored in hbase for that record is a `commit` on the timeline. This makes any record that is stored to hbase index during a `deltacommit` considered an invalid index and treated as a new record. This causes the hbase index to be updated every time which leads to records being able to be in multiple partitions and even in different file groups within same partition.
   
   ## Brief change log
   
   * Modify HbaseIndex.checkIfValidCommit to consider DELTA_COMMIT timestamp as valid index timestamp
   
   ## Verify this pull request
   
   This change added tests and can be verified as follows:
   
     - Verified test failed on MOR table before change and succeeding now
     - Manually verified by:
          * Uploaded patched JAR to EMR 5.30.1 cluster
          * Create MOR table w/ HBASE index
          * Upsert record
          * Upsert record with new partition
          * Validate new partition was not created and existing partition displayed update to record


----------------------------------------------------------------
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

[GitHub] [hudi] n3nash commented on a change in pull request #1963: [HUDI-1188] Hbase index MOR tables records not being deduplicated

Posted by GitBox <gi...@apache.org>.
n3nash commented on a change in pull request #1963:
URL: https://github.com/apache/hudi/pull/1963#discussion_r470768575



##########
File path: hudi-client/src/main/java/org/apache/hudi/index/hbase/HBaseIndex.java
##########
@@ -182,6 +182,7 @@ private boolean checkIfValidCommit(HoodieTableMetaClient metaClient, String comm
     // 2) is less than the first commit ts in the timeline
     return !commitTimeline.empty()
         && (commitTimeline.containsInstant(new HoodieInstant(false, HoodieTimeline.COMMIT_ACTION, commitTs))

Review comment:
       Yes, @rmpifer please use the above API Vinoth mentioned to refactor this code so we can include both DELTA_COMMIT & COMMIT actions




----------------------------------------------------------------
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

[GitHub] [hudi] rmpifer commented on a change in pull request #1963: [HUDI-1188] Hbase index MOR tables records not being deduplicated

Posted by GitBox <gi...@apache.org>.
rmpifer commented on a change in pull request #1963:
URL: https://github.com/apache/hudi/pull/1963#discussion_r473247387



##########
File path: hudi-client/src/main/java/org/apache/hudi/index/hbase/HBaseIndex.java
##########
@@ -177,13 +176,11 @@ private Get generateStatement(String key) throws IOException {
   }
 
   private boolean checkIfValidCommit(HoodieTableMetaClient metaClient, String commitTs) {
-    HoodieTimeline commitTimeline = metaClient.getActiveTimeline().filterCompletedInstants();
+    HoodieTimeline commitTimeline = metaClient.getCommitsTimeline().filterCompletedInstants();

Review comment:
       @n3nash It looks like HoodieTableMetaClient.getCommitsTimeline internally calls getActiveTimeline()
   https://github.com/rmpifer/hudi/blob/master/hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java#L478




----------------------------------------------------------------
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

[GitHub] [hudi] rmpifer commented on a change in pull request #1963: [HUDI-1188] Hbase index MOR tables records not being deduplicated

Posted by GitBox <gi...@apache.org>.
rmpifer commented on a change in pull request #1963:
URL: https://github.com/apache/hudi/pull/1963#discussion_r471915571



##########
File path: hudi-client/src/main/java/org/apache/hudi/index/hbase/HBaseIndex.java
##########
@@ -182,6 +182,7 @@ private boolean checkIfValidCommit(HoodieTableMetaClient metaClient, String comm
     // 2) is less than the first commit ts in the timeline
     return !commitTimeline.empty()
         && (commitTimeline.containsInstant(new HoodieInstant(false, HoodieTimeline.COMMIT_ACTION, commitTs))

Review comment:
       Thanks for the suggestion. Updated to address comment




----------------------------------------------------------------
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

[GitHub] [hudi] n3nash commented on a change in pull request #1963: [HUDI-1188] Hbase index MOR tables records not being deduplicated

Posted by GitBox <gi...@apache.org>.
n3nash commented on a change in pull request #1963:
URL: https://github.com/apache/hudi/pull/1963#discussion_r473362342



##########
File path: hudi-client/src/main/java/org/apache/hudi/index/hbase/HBaseIndex.java
##########
@@ -177,13 +176,11 @@ private Get generateStatement(String key) throws IOException {
   }
 
   private boolean checkIfValidCommit(HoodieTableMetaClient metaClient, String commitTs) {
-    HoodieTimeline commitTimeline = metaClient.getActiveTimeline().filterCompletedInstants();
+    HoodieTimeline commitTimeline = metaClient.getCommitsTimeline().filterCompletedInstants();

Review comment:
       okay, great, thanks for checking.




----------------------------------------------------------------
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

[GitHub] [hudi] n3nash commented on a change in pull request #1963: [HUDI-1188] Hbase index MOR tables records not being deduplicated

Posted by GitBox <gi...@apache.org>.
n3nash commented on a change in pull request #1963:
URL: https://github.com/apache/hudi/pull/1963#discussion_r472687401



##########
File path: hudi-client/src/main/java/org/apache/hudi/index/hbase/HBaseIndex.java
##########
@@ -177,13 +176,11 @@ private Get generateStatement(String key) throws IOException {
   }
 
   private boolean checkIfValidCommit(HoodieTableMetaClient metaClient, String commitTs) {
-    HoodieTimeline commitTimeline = metaClient.getActiveTimeline().filterCompletedInstants();
+    HoodieTimeline commitTimeline = metaClient.getCommitsTimeline().filterCompletedInstants();

Review comment:
       Can you please change this to -> `metaClient.getActiveTimeline().getCommitsTimeline()`. For context, the getActiveTimeline() loads the timeline once and then caches it. The getCommitsTimeline() call will not force this, in this case it's working by coincidence because the metaClient passed already has the loaded timeline from the caller but I would want to avoid having that assumption. 
   @rmpifer 




----------------------------------------------------------------
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

[GitHub] [hudi] vinothchandar commented on a change in pull request #1963: [HUDI-1188] Hbase index MOR tables records not being deduplicated

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #1963:
URL: https://github.com/apache/hudi/pull/1963#discussion_r470431566



##########
File path: hudi-client/src/main/java/org/apache/hudi/index/hbase/HBaseIndex.java
##########
@@ -182,6 +182,7 @@ private boolean checkIfValidCommit(HoodieTableMetaClient metaClient, String comm
     // 2) is less than the first commit ts in the timeline
     return !commitTimeline.empty()
         && (commitTimeline.containsInstant(new HoodieInstant(false, HoodieTimeline.COMMIT_ACTION, commitTs))

Review comment:
       we could use the `metaClient.getCommitsTimeline()` or one of those methods which will automatically determine the commit instant type based on table type?




----------------------------------------------------------------
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

[GitHub] [hudi] vinothchandar commented on pull request #1963: [HUDI-1188] Hbase index MOR tables records not being deduplicated

Posted by GitBox <gi...@apache.org>.
vinothchandar commented on pull request #1963:
URL: https://github.com/apache/hudi/pull/1963#issuecomment-673908103


   @n3nash can you please review this. (we can land after the release branch is cut) 


----------------------------------------------------------------
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

[GitHub] [hudi] n3nash merged pull request #1963: [HUDI-1188] Hbase index MOR tables records not being deduplicated

Posted by GitBox <gi...@apache.org>.
n3nash merged pull request #1963:
URL: https://github.com/apache/hudi/pull/1963


   


----------------------------------------------------------------
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