[HUDI-9330] Avoid storing the clean plan in inflight clean instant #13192
Conversation
1. avoid storing the clean plan in inflight clean instant Signed-off-by: TheR1sing3un <[email protected]>
1. delete empty clean instant Signed-off-by: TheR1sing3un <[email protected]>
1. fix test Signed-off-by: TheR1sing3un <[email protected]>
1. fix test Signed-off-by: TheR1sing3un <[email protected]>
| @@ -257,7 +257,8 @@ public HoodieCleanMetadata execute() { | |||
| } | |||
|
|
|||
| for (HoodieInstant hoodieInstant : pendingCleanInstants) { | |||
| if (table.getCleanTimeline().isEmpty(hoodieInstant)) { | |||
| if (table.getCleanTimeline().isEmpty(CleanerUtils.getCleanRequestInstant(table.getMetaClient(), hoodieInstant))) { | |||
| // remove the empty instant | |||
There was a problem hiding this comment.
we also need to delete the empty inflight file?
There was a problem hiding this comment.
we also need to delete the empty inflight file?
The original logic here is to delete the inflight file. I think what you should be saying is whether we also need to delete the request file, right?
I originally attempted to make this change, but I found that it would disrupt the existing ut logic, you can refer to my previous commit: 16230a4. To reduce the unknown risks brought by the change, this change only makes alterations to the way the plan is obtained and does not change any other logic.
There was a problem hiding this comment.
we also need to delete the empty inflight file?
Also, we can no longer perform the isEmpty judgment on the inflight file now because this file no longer stores the clean plan. Its size is always 0 and the isEmpty method is always true.
| @@ -168,10 +168,18 @@ public static Option<HoodieInstant> getEarliestCommitToRetain( | |||
| public static HoodieCleanerPlan getCleanerPlan(HoodieTableMetaClient metaClient, HoodieInstant cleanInstant) | |||
| throws IOException { | |||
| CleanPlanMigrator cleanPlanMigrator = new CleanPlanMigrator(metaClient); | |||
| cleanInstant = getCleanRequestInstant(metaClient, cleanInstant); | |||
There was a problem hiding this comment.
can we make the cleanInstant as REQUESTED from the invoker?
There was a problem hiding this comment.
can we make the
cleanInstantasREQUESTEDfrom the invoker?
The current invoker does not care whether the incoming Instant is request or inflight. To be compatible with the current usage logic of invoker, I choose to perform the instant conversion at this method. Moreover, from the design of this method itself, given a clean instant of any state, It is reasonable that we should all be able to return to the correct plan.
There was a problem hiding this comment.
so there is possibility an empty inflight instant been removed for 2 times.
There was a problem hiding this comment.
so there is possibility an empty inflight instant been removed for 2 times.
If there is a concurrent cleaner, it is indeed possible, but this behavior has nothing to do with the current code modification
There was a problem hiding this comment.
Can we check all the callers of it, I see several places have already do some pre-transformation to make sure the instant is a REQUESTED before calling it.
There was a problem hiding this comment.
Can we check all the callers of it, I see several places have already do some pre-transformation to make sure the instant is a REQUESTED before calling it.
do we remove the unnecessary requested instant creation before the invocation of the method.
| HoodieCleanerPlan cleanerPlan = metaClient.getActiveTimeline().readCleanerPlan(cleanInstant); | ||
| return cleanPlanMigrator.upgradeToLatest(cleanerPlan, cleanerPlan.getVersion()); | ||
| } | ||
|
|
||
| public static HoodieInstant getCleanRequestInstant(HoodieTableMetaClient metaClient, HoodieInstant cleanInstant) { | ||
| if (cleanInstant.isInflight() || cleanInstant.isCompleted()) { |
There was a problem hiding this comment.
we can check if it is not REQUESTED instead.
There was a problem hiding this comment.
we can check if it is not
REQUESTEDinstead.
Done~
1. minor refactor Signed-off-by: TheR1sing3un <[email protected]>
|
@danny0405 Hi, Danny, is there anything else in this pr that needs to be reviewed? |
|
@hudi-bot run azure |
It is not necessary for hudi to store a clean plan in the inflight clean instant as well. This will increase the storage waste of our metadata directory.
Clean plan can always be fetched from request clean instant.
Change Logs
Impact
reduce storage waste about timeline
Risk level (write none, low medium or high below)
low
Documentation Update
none
Contributor's checklist