[HUDI-6040]Stop writing and reading compaction plans from .aux folder #8385

Mulavar · 2023-04-05T07:48:52Z

Change Logs

Stop writing and reading compaction plans from .aux folder
Describe context and summary for this change. Highlight if any code was copied.

Impact

Reduce I/O when schedule compaction from 2 to 1.
Describe any public API or user-facing feature change or any performance impact.

Risk level (write none, low medium or high below)

None.
If medium or high, explain what verification was done to mitigate the risks.

Documentation Update

None
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 to make
changes to the website.

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

danny0405

+1, nice catch ~

danny0405 · 2023-04-06T14:38:09Z

hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java

  public void saveToCompactionRequested(HoodieInstant instant, Option<byte[]> content, boolean overwrite) {
    ValidationUtils.checkArgument(instant.getAction().equals(HoodieTimeline.COMPACTION_ACTION));
-    // Write workload to auxiliary folder
-    createFileInAuxiliaryFolder(instant, content);


cc @bvaradar for taking over this, generally looks good.

Mulavar · 2023-04-07T02:52:29Z

@bvaradar Hi, could u take the time to review this pr, Is there anything I need to modify?

bvaradar

@Mulavar : This requires a table version change also as we need to create .aux files when we downgrade to older version. Can you add relevant UpgradeDowngrade handlers for this.

Mulavar · 2023-04-11T06:20:09Z

@bvaradar Thanks for your review. I think your suggestion is very valuable, and I need to read that piece of code first because I'm not very familiar with the code of table version downgrade.

Mulavar · 2023-04-11T11:16:38Z

@bvaradar hi bvaradar, could u take some time to review the pr again?

bvaradar · 2023-04-12T01:27:42Z

@Mulavar : We need to create a higher version 6 and write upgrade/downgrade to handle transition from current version (5). You can look at #6248 as example of how to do this.

Mulavar · 2023-04-12T04:41:19Z

@Mulavar : This requires a table version change also as we need to create .aux files when we downgrade to older version. Can you add relevant UpgradeDowngrade handlers for this.

@bvaradar thanks, done.

bvaradar

Realized there are more places to cleanup. Please take a look.

.../hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/FiveToSixUpgradeHandler.java

bvaradar · 2023-04-12T19:24:30Z

.../hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/FiveToSixUpgradeHandler.java

+    compactionTimeline.getInstants().stream().forEach(instant -> {
+      String fileName = instant.getFileName();
+      try {
+        metaClient.getFs().delete(new Path(metaClient.getMetaAuxiliaryPath(), fileName));


@Mulavar : We need to basically look at all caller of metaClient.getMetaAuxiliaryPath() and remove the usage of metaAuxiliaryPath since we should not no longer write to them. Also in HoodieTableMetaClient.initTableAndGetMetaClient, can you remove the creation of .aux folder

I found that some other feature like bootstrap index also use the .aux folder, refer to org.apache.hudi.common.table.HoodieTableMetaClient#BOOTSTRAP_INDEX_ROOT_FOLDER_PATH and we can not just remove the creation of .aux folder. Maybe we should remove the caller of metaClient.getMetaAuxiliaryPath() related to compaction logic?

Yes, Good point. We should only cleanup compaction plans in .aux folder and remove compaction only callers

...-client/hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/UpgradeDowngrade.java

bvaradar · 2023-04-16T17:40:47Z

.../hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/FiveToSixUpgradeHandler.java

+    compactionTimeline.getInstants().stream().forEach(instant -> {
+      String fileName = instant.getFileName();
+      try {
+        metaClient.getFs().delete(new Path(metaClient.getMetaAuxiliaryPath(), fileName));


Yes, Good point. We should only cleanup compaction plans in .aux folder and remove compaction only callers

bvaradar

@Mulavar : Thanks for cleaning this. Functionality wise, it looks good to me. Can you add detailed tests for TestFiveSixUpgradeDowngrader. We need to cover cases when (a) aux folder is empty (b) aux folder is non-empty with incomplete compactions and (c) case b along with metadata bootstrap done. In all 3 cases, can you run subsequent compaction and check compaction succeeded followed by a write-schedule_compaction-run_compaction. Similar cases needed for downgrade. This should cover all cases.

@danny0405 : There is some cleanup in Flink related code. Can you kindly take a look and give a thumbs up if it looks good.

Mulavar · 2023-04-17T02:40:07Z

@bvaradar Yeah bvaradar, I agreed with you that we need to add some test cases to verify the upgrader/downgrader, but I'm not sure if there's a need to run subsequent compaction, cause we do not change anything about the compaction plan content, so maybe we should better verify if we can still read the compaction plan from .hoodie folder when after deleting compaction.requested from .aux folder. Since the upgrade verification has been covered by org.apache.hudi.common.table.timeline.TestHoodieActiveTimeline#testLoadingInstantsFromFiles and we could add some tests to verify SixToFiveDowngradeHandler. WDYT?

Mulavar · 2023-04-18T02:22:19Z

@hudi-bot run azure

bvaradar · 2023-04-20T03:03:05Z

@Mulavar : W.r.t running compaction after upgrade/downgrade, this will help prove that the upgrade will be safer if we can ensure compaction did not fail in any place after upgrade/downgrade.

Mulavar · 2023-04-20T03:24:43Z

@bvaradar Yeah, but I'm not very familiar with the compaction test, could you give me some tips on which example should I look at?

Mulavar · 2023-04-20T03:27:04Z

@hudi-bot run azure

...source/hudi-flink/src/test/java/org/apache/hudi/sink/compact/ITTestHoodieFlinkCompactor.java

nsivabalan

Should we completely remove all usages of "aux" folder with this patch?

I don't see any other usages after this patch.

Also, @bvaradar : can you help me understand what will happen in this case.
if a compaction plan was written to aux but failed to add to ".hoodie" just before upgrade.
after upgrade, if we delete the entire "aux" folder, we should be good right. there won't be any other traces of that failed compaction schedule ?

.../hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/FiveToSixUpgradeHandler.java

nsivabalan · 2023-04-25T02:07:09Z

.../hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/FiveToSixUpgradeHandler.java

+    HoodieTableMetaClient metaClient = table.getMetaClient();
+    // delete compaction file from .aux
+    HoodieTimeline compactionTimeline = metaClient.getActiveTimeline().filterPendingCompactionTimeline()
+        .filter(instant -> instant.getState() == HoodieInstant.State.REQUESTED);


I checked the usages of aux folder and its used only incase of compaction plans and may be flink clustering.
why can't we delete entire aux folder ?

"I found that some other features like bootstrap index also use the .aux folder, refer to org.apache.hudi.common.table.HoodieTableMetaClient#BOOTSTRAP_INDEX_ROOT_FOLDER_PATH and we can not just remove the creation of .aux folder. Maybe we should remove the caller of metaClient.getMetaAuxiliaryPath() related to compaction logic?"
This patch only cares about the compaction, and there is some other file/folder in the .aux folder, we should only remove the files about compaction. If we want to confirm if the entire .aux folder could be deleted, maybe better if we should open another issue to discuss it.

nsivabalan · 2023-04-25T02:08:49Z

...udi-client-common/src/main/java/org/apache/hudi/table/upgrade/SixToFiveDowngradeHandler.java

+    HoodieTable table = upgradeDowngradeHelper.getTable(config, context);
+    HoodieTableMetaClient metaClient = table.getMetaClient();
+    // sync compaction requested file to .aux
+    HoodieTimeline compactionTimeline = metaClient.getActiveTimeline().filterPendingCompactionTimeline()


this could return instants either in inflight or in requested. if its inflight, we need to fetch the filename by constructing the requested instant as well

the code snippet has been followed by the filter which does the thing you said.

what I meant is, for a compaction that has both requested and inflight in ".hoodie", only INFLIGHT will be returned with this call (metaClient.getActiveTimeline().filterPendingCompactionTimeline()) and in subsequent like you are filtering only for requested and hence we might miss.
can you check please.

Thanks, nice catch! fixed.

nsivabalan · 2023-04-25T02:12:48Z

we should also https://issues.apache.org/jira/browse/HUDI-546 along with 6040.

…ndler

bvaradar · 2023-04-25T05:06:51Z

Should we completely remove all usages of "aux" folder with this patch?

I don't see any other usages after this patch.

Also, @bvaradar : can you help me understand what will happen in this case. if a compaction plan was written to aux but failed to add to ".hoodie" just before upgrade. after upgrade, if we delete the entire "aux" folder, we should be good right. there won't be any other traces of that failed compaction schedule ?

@nsivabalan : It is still used for bootstrap. Bootstrap folder is under aux
Regarding the partial write case, it should be fine since we rely on timeline (.hoodie/) to figure out pending compactions. In both upgrade and normal case, this would be as if no compaction is pending.

danny0405 · 2023-04-25T05:10:56Z

It is still used for bootstrap. Bootstrap folder is under aux
Regarding the partial write case, it should be fine since we rely on timeline (.hoodie/) to figure out pending compactions. In both upgrade and normal case, this would be as if no compaction is pending.

Flink actually uses the .aux folder for some configurations like 1. fs view storage conf and 2. checkpoint metadata, so we may need to keep this folder, just drop the file creation for compaction should be fine.

…ght in ".hoodie"

Mulavar · 2023-04-25T13:26:14Z

@nsivabalan Hi nsivabalan, I've fixed the comment, could u check this patch again~

hudi-bot · 2023-04-25T15:18:55Z

CI report:

3874447 UNKNOWN
0f1555e Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

bvaradar · 2023-04-28T08:25:47Z

@nsivabalan : Are you ok with this change ? If so, can you approve it ?

...udi-client-common/src/main/java/org/apache/hudi/table/upgrade/SixToFiveDowngradeHandler.java

danny0405 reviewed Apr 6, 2023

View reviewed changes

bvaradar self-assigned this Apr 7, 2023

bvaradar requested changes Apr 11, 2023

View reviewed changes

Mulavar force-pushed the HUDI-6040 branch from d663835 to 19ff36f Compare April 11, 2023 10:12

bvaradar requested changes Apr 12, 2023

View reviewed changes

Mulavar force-pushed the HUDI-6040 branch 2 times, most recently from 54fbf42 to 6fd073a Compare April 13, 2023 03:00

bvaradar reviewed Apr 16, 2023

View reviewed changes

bvaradar requested changes Apr 16, 2023

View reviewed changes

Mulavar force-pushed the HUDI-6040 branch 2 times, most recently from ddb5108 to 1cd0db6 Compare April 17, 2023 02:44

Mulavar force-pushed the HUDI-6040 branch from 1cd0db6 to bfe8f90 Compare April 20, 2023 02:45

[HUDI-6040]Stop writing compaction plans to .aux folder

1b03f49

Mulavar force-pushed the HUDI-6040 branch 2 times, most recently from 9149994 to 9659d42 Compare April 23, 2023 16:01

Mulavar commented Apr 24, 2023

View reviewed changes

...source/hudi-flink/src/test/java/org/apache/hudi/sink/compact/ITTestHoodieFlinkCompactor.java Show resolved Hide resolved

bvaradar reviewed Apr 24, 2023

View reviewed changes

...source/hudi-flink/src/test/java/org/apache/hudi/sink/compact/ITTestHoodieFlinkCompactor.java Outdated Show resolved Hide resolved

nsivabalan requested changes Apr 25, 2023

View reviewed changes

[HUDI-6040]add upgrade/downgrade test with flink compaction

8f14d7a

Mulavar force-pushed the HUDI-6040 branch from 9659d42 to 8f14d7a Compare April 25, 2023 02:15

[HUDI-6040]add a more detailed description about upgrade/downgrade ha…

83a903b

…ndler

bvaradar approved these changes Apr 25, 2023

View reviewed changes

[HUDI-6040]fix downgrade handler when it has both requested and infli…

0f1555e

…ght in ".hoodie"

danny0405 approved these changes Apr 26, 2023

View reviewed changes

nsivabalan reviewed May 3, 2023

View reviewed changes

...udi-client-common/src/main/java/org/apache/hudi/table/upgrade/SixToFiveDowngradeHandler.java Show resolved Hide resolved

nsivabalan approved these changes May 9, 2023

View reviewed changes

danny0405 merged commit 8db07a8 into apache:master May 9, 2023

Mulavar deleted the HUDI-6040 branch May 10, 2023 03:11

[HUDI-6040]Stop writing and reading compaction plans from .aux folder #8385

[HUDI-6040]Stop writing and reading compaction plans from .aux folder #8385

Uh oh!

Conversation

Mulavar commented Apr 5, 2023

Change Logs

Impact

Risk level (write none, low medium or high below)

Documentation Update

Contributor's checklist

Uh oh!

danny0405 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mulavar commented Apr 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bvaradar left a comment

Choose a reason for hiding this comment

Uh oh!

Mulavar commented Apr 11, 2023

Uh oh!

Mulavar commented Apr 11, 2023

Uh oh!

bvaradar commented Apr 12, 2023

Uh oh!

Mulavar commented Apr 12, 2023

Uh oh!

bvaradar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mulavar Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaradar left a comment

Choose a reason for hiding this comment

Uh oh!

Mulavar commented Apr 17, 2023

Uh oh!

Mulavar commented Apr 18, 2023

Uh oh!

bvaradar commented Apr 20, 2023

Uh oh!

Mulavar commented Apr 20, 2023

Uh oh!

Mulavar commented Apr 20, 2023

Uh oh!

Uh oh!

Uh oh!

nsivabalan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsivabalan Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mulavar commented Apr 7, 2023 •

edited

Loading

Mulavar Apr 13, 2023 •

edited

Loading

nsivabalan Apr 25, 2023 •

edited

Loading