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

Posted to commits@hudi.apache.org by "Zouxxyy (via GitHub)" <gi...@apache.org> on 2023/04/03 08:09:20 UTC

[GitHub] [hudi] Zouxxyy opened a new pull request, #8364: [HUDI-6007] Add log files to savepoint metadata

Zouxxyy opened a new pull request, #8364:
URL: https://github.com/apache/hudi/pull/8364

   ### Change Logs
   
   _Describe context and summary for this change. Highlight if any code was copied._
   
   ### Impact
   
   _Describe any public API or user-facing feature change or any performance impact._
   
   ### Risk level (write none, low medium or high below)
   
   _If medium or high, explain what verification was done to mitigate the risks._
   
   ### Documentation Update
   
   _Describe any necessary documentation update if there is any new feature, config, or user-facing change_
   
   - _The config description must be updated if new configs are added or the default value of the configs are changed_
   - _Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
     ticket number here and follow the [instruction](https://hudi.apache.org/contribute/developer-setup#website) to make
     changes to the website._
   
   ### Contributor's checklist
   
   - [ ] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
   - [ ] Change Logs and Impact were stated clearly
   - [ ] Adequate tests were added if applicable
   - [ ] CI passed
   


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

To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org

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

[GitHub] [hudi] danny0405 commented on a diff in pull request #8364: [HUDI-6007] Add log files to savepoint metadata

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.

danny0405 commented on code in PR #8364:
URL: https://github.com/apache/hudi/pull/8364#discussion_r1188294647


##########
hudi-common/src/main/java/org/apache/hudi/common/table/view/AbstractTableFileSystemView.java:
##########
@@ -850,6 +850,25 @@ public final Stream<FileSlice> getLatestFileSlicesBeforeOrOn(String partitionStr
     }
   }
 
+  @Override
+  public final Map<String, Stream<FileSlice>> getAllLatestFileSlicesBeforeOrOn(String maxCommitTime) {
+    try {
+      readLock.lock();
+      List<String> formattedPartitionList = ensureAllPartitionsLoadedCorrectly();
+      return formattedPartitionList.stream().collect(Collectors.toMap(
+          Function.identity(),
+          partitionPath -> fetchAllStoredFileGroups(partitionPath)
+              .filter(slice -> !isFileGroupReplacedBeforeOrOn(slice.getFileGroupId(), maxCommitTime))
+              .map(fg -> fg.getAllFileSlicesBeforeOn(maxCommitTime))
+              .map(sliceStream -> sliceStream.flatMap(slice -> this.filterBaseFileAfterPendingCompaction(slice, false)))
+              .map(sliceStream -> Option.fromJavaOptional(sliceStream.findFirst())).filter(Option::isPresent).map(Option::get)

Review Comment:
   > this.filterBaseFileAfterPendingCompaction
   
   This may give in-complete slice for the `FileGroup`, but it still works for your use case. A more general way is we merge the pending slice log files with the previous file slice logs, just like what we do for the reader view.



##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##########
@@ -215,6 +216,21 @@ private List<String> getPartitionPathsForFullCleaning() {
     return FSUtils.getAllPartitionPaths(context, config.getMetadataConfig(), config.getBasePath());
   }
 
+  /**
+   *  Verify whether file slice exists in savepointedFiles, check both base file and log files
+   */
+  private boolean isFsExistInSavepointedFiles(FileSlice fs, List<String> savepointedFiles) {
+    if (fs.getBaseFile().isPresent() && savepointedFiles.contains(fs.getBaseFile().get().getFileName())) {
+      return true;
+    }

Review Comment:
   isFsExistInSavepointedFiles -> isFileSliceExistInSavepointedFiles



##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/TestMergeIntoLogOnlyTable.scala:
##########
@@ -88,4 +88,57 @@ class TestMergeIntoLogOnlyTable extends HoodieSparkSqlTestBase {
       )
     })
   }
+
+  test("Test Savepoint with Log Only MOR Table") {
+    withRecordType()(withTempDir { tmp =>
+      // Create table with INMEMORY index to generate log only mor table.
+      val tableName = generateTableName
+      spark.sql(
+        s"""
+           |create table $tableName (
+           |  id int,

Review Comment:
   Can we move the tests to `TestSavepointsProcedure`



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

To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org

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

[GitHub] [hudi] danny0405 merged pull request #8364: [HUDI-6007] Add log files to savepoint metadata

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.

danny0405 merged PR #8364:
URL: https://github.com/apache/hudi/pull/8364


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

To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org

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

[GitHub] [hudi] Zouxxyy commented on a diff in pull request #8364: [HUDI-6007] Add log files to savepoint metadata

Posted by "Zouxxyy (via GitHub)" <gi...@apache.org>.

Zouxxyy commented on code in PR #8364:
URL: https://github.com/apache/hudi/pull/8364#discussion_r1188679145


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##########
@@ -215,6 +216,21 @@ private List<String> getPartitionPathsForFullCleaning() {
     return FSUtils.getAllPartitionPaths(context, config.getMetadataConfig(), config.getBasePath());
   }
 
+  /**
+   *  Verify whether file slice exists in savepointedFiles, check both base file and log files
+   */
+  private boolean isFsExistInSavepointedFiles(FileSlice fs, List<String> savepointedFiles) {
+    if (fs.getBaseFile().isPresent() && savepointedFiles.contains(fs.getBaseFile().get().getFileName())) {
+      return true;
+    }

Review Comment:
   A finer-grained clean should be achievable, as long as our savepoint is accurate enough, but I'm not sure if it will affects other. If we want to achieve it, we should open a new PR?



##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##########
@@ -215,6 +216,21 @@ private List<String> getPartitionPathsForFullCleaning() {
     return FSUtils.getAllPartitionPaths(context, config.getMetadataConfig(), config.getBasePath());
   }
 
+  /**
+   *  Verify whether file slice exists in savepointedFiles, check both base file and log files
+   */
+  private boolean isFsExistInSavepointedFiles(FileSlice fs, List<String> savepointedFiles) {
+    if (fs.getBaseFile().isPresent() && savepointedFiles.contains(fs.getBaseFile().get().getFileName())) {
+      return true;
+    }

Review Comment:
   A finer-grained clean should be achievable, as long as our savepoint is accurate enough, but I'm not sure if it will affects other. If we want to achieve it, we should open a new different PR?



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

To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org

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

[GitHub] [hudi] Zouxxyy commented on a diff in pull request #8364: [HUDI-6007] Add log files to savepoint metadata

Posted by "Zouxxyy (via GitHub)" <gi...@apache.org>.

Zouxxyy commented on code in PR #8364:
URL: https://github.com/apache/hudi/pull/8364#discussion_r1188679145


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##########
@@ -215,6 +216,21 @@ private List<String> getPartitionPathsForFullCleaning() {
     return FSUtils.getAllPartitionPaths(context, config.getMetadataConfig(), config.getBasePath());
   }
 
+  /**
+   *  Verify whether file slice exists in savepointedFiles, check both base file and log files
+   */
+  private boolean isFsExistInSavepointedFiles(FileSlice fs, List<String> savepointedFiles) {
+    if (fs.getBaseFile().isPresent() && savepointedFiles.contains(fs.getBaseFile().get().getFileName())) {
+      return true;
+    }

Review Comment:
   A finer-grained clean should be achievable, as long as our savepoint is accurate enough, but I'm not sure if it will affects other



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

To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org

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

[GitHub] [hudi] danny0405 commented on a diff in pull request #8364: [HUDI-6007] Add log files to savepoint metadata

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.

danny0405 commented on code in PR #8364:
URL: https://github.com/apache/hudi/pull/8364#discussion_r1188268883


##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##########
@@ -215,6 +216,21 @@ private List<String> getPartitionPathsForFullCleaning() {
     return FSUtils.getAllPartitionPaths(context, config.getMetadataConfig(), config.getBasePath());
   }
 
+  /**
+   *  Verify whether file slice exists in savepointedFiles, check both base file and log files
+   */
+  private boolean isFsExistInSavepointedFiles(FileSlice fs, List<String> savepointedFiles) {
+    if (fs.getBaseFile().isPresent() && savepointedFiles.contains(fs.getBaseFile().get().getFileName())) {
+      return true;
+    }

Review Comment:
   Should we return true if base file exists while log file is missing?



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

To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org

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

[GitHub] [hudi] boundarymate commented on pull request #8364: [HUDI-6007] Add log files to savepoint metadata

Posted by "boundarymate (via GitHub)" <gi...@apache.org>.

boundarymate commented on PR #8364:
URL: https://github.com/apache/hudi/pull/8364#issuecomment-1500059693

   @danny0405 Can you help with a review~


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

To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org

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