[HUDI-137] Fix state transitions for Hudi cleaning action #942
Conversation
bvaradar
changed the title
hudi-client/src/main/java/org/apache/hudi/HoodieWriteClient.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Should this snippet of code removed?
There was a problem hiding this comment.
Will confirm with @n3nash before I remove this part.
|
Thanks for opening the PR @bvaradar , left a few comments. |
bvaradar
force-pushed
the
hudi_137_clean
branch
from
6d2aea7 to
1e6b8a8
Compare
There was a problem hiding this comment.
can we create a HoodieCleanerClient and consolidate all the code around cleaning there? in the interest of keeping HoodieWriteClient better readable
There was a problem hiding this comment.
can we pass the table from clean() above?
There was a problem hiding this comment.
is your formatting consistent? this line could easily fit on the previous line, looks like?
There was a problem hiding this comment.
Will run spotcheck to fix this.
There was a problem hiding this comment.
space between , and 1 ? is checkstyle not enforcing this .
There was a problem hiding this comment.
in the real code, we could have the same instant time for commit and clean? is this needed
There was a problem hiding this comment.
IIRC, I added this to force different timestamps for same actions (1.clean, 2.clean,...)
There was a problem hiding this comment.
what is your thought on moving forward and skipping the requested -> inflight transition if it was already in inflight instead of reverting back to state? do we do the same thing for compaction
There was a problem hiding this comment.
Agree, In the case of compaction, we do this and also rollback partially written parquet. In this case, it is not needed.
There was a problem hiding this comment.
Since we are finishing up any inflight/requested cleans first before getting here, this list to clean here would do mostly pick only actual new files to clean ?
bvaradar
force-pushed
the
hudi_137_clean
branch
from
1e6b8a8 to
8e393d9
Compare
|
@vinothchandar : Addressed review comments. Also, re-enabled spotless to fix any linting automatically and disabled spotless again. We can wait for the tests to pass before reviewing again. |
bvaradar
force-pushed
the
hudi_137_clean
branch
5 times, most recently
from
8bf4d20 to
aaa73d5
Compare
|
@vinothchandar: Updated PR. Please review when you get a chance |
vinothchandar
left a comment
vinothchandar
left a comment
There was a problem hiding this comment.
Please fix the two small things and go ahead and merge
There was a problem hiding this comment.
please move these off code and into a JIRA..
There was a problem hiding this comment.
yes. we can revisit everything more holistically for archival redesign
There was a problem hiding this comment.
There was a problem hiding this comment.
Should this be refreshed for every operation like the table? or not needed since table is passed in.. I think latter.. just calling out to confirm
There was a problem hiding this comment.
Yes, it is the latter. No need to refresh the Cleaner client itself.
…action actions Before this change, Cleaner performs cleaning of old file versions and then stores the deleted files in .clean files. With this setup, we will not be able to track file deletions if a cleaner fails after deleting files but before writing .clean metadata. This is fine for regular file-system view generation but Incremental timeline syncing relies on clean/commit/compaction metadata to keep a consistent file-system view. Cleaner state transitions is now similar to that of compaction. 1. Requested : HoodieWriteClient.scheduleClean() selects the list of files that needs to be deleted and stores them in metadata 2. Inflight : HoodieWriteClient marks the state to be inflight before it starts deleting 3. Completed : HoodieWriteClient marks the state after completing the deletion according to the cleaner plan
bvaradar
force-pushed
the
hudi_137_clean
branch
from
aaa73d5 to
7d7bddb
Compare
…locks with timestamps > maxInstant times (apache#942)
…locks with timestamps > maxInstant times (apache#942)
…locks with timestamps > maxInstant times (apache#942)
…locks with timestamps > maxInstant times (apache#942)
…locks with timestamps > maxInstant times (apache#942)
3 participants
Before this change, Cleaner performs cleaning of old file versions and then stores the deleted files in .clean files.
With this setup, we will not be able to track file deletions if a cleaner fails after deleting files but before writing .clean metadata.
This is fine for regular file-system view generation but Incremental timeline syncing relies on clean/commit/compaction metadata to keep a consistent file-system view.
Cleaner state transitions is now similar to that of compaction.
There will be followup PRs after this :