-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HUDI-8248] Fixing Log Record reader to include rollback blocks with timestamps > maxInstant times #12033
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Oct 1, 2024
danny0405
approved these changes
Oct 2, 2024
Looks good, let's fix the test failures. |
yihua
requested changes
Oct 2, 2024
hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordReader.java
Show resolved
Hide resolved
nsivabalan
force-pushed
the
logRecordReaderRollbackFix
branch
from
October 2, 2024 23:13
d5b6fc4
to
ee8d6e0
Compare
github-actions
bot
added
size:M
PR with lines of changes in (100, 300]
and removed
size:L
PR with lines of changes in (300, 1000]
labels
Oct 3, 2024
nsivabalan
force-pushed
the
logRecordReaderRollbackFix
branch
from
October 3, 2024 21:20
1ece130
to
14ffdd1
Compare
…locks with timestamps > maxInstant times (apache#930) Co-authored-by: Y Ethan Guo <[email protected]>
…locks with timestamps > maxInstant times (apache#942)
…ot receive partially commit log files at all
nsivabalan
force-pushed
the
logRecordReaderRollbackFix
branch
from
October 4, 2024 02:10
14ffdd1
to
72f0ab3
Compare
yihua
approved these changes
Oct 5, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Logs
Impact
LogRecordReader takes in a maxInstant time beyond which log blocks are ignored. But w/ rollbacks, there are chances it could lead to data consistency issues.
Lets go through an illustration:
Say, we have t1.dc, t2.dc and t2.dc crashed mid way.
Current layout is,
Then we start t5.dc say. just when we start t5.dc, hudi detects pending commit and triggers a rollback. And this rollback will get an instant time of t6 (t6.rb). Note that rollback's commit time is greater than t5 or current ongoing delta commit.
So, once rollback completes, this is the layout.
And once t5.dc completes, this is how the layout looks like
At this point in time, when we trigger snapshot read or try to trigger tagLocation w/ global index, maxInstant is set to last entry among commits timeline which is t5. So, while LogRecordReader while processing all log blocks, when it reaches lf3, it detects the timestamp of t6 > t5 (i.e max instant time) and bails out of for loop. So, in essence it may even read lf4 in above scenario.
This patch attempts the fix the same.
Fix:
We only hold the constraint true for any data blocks and relax it for other block types. For eg, any data blocks > max instant time will be ignored. But all rollback blocks are taken into consideration.
Risk level (write none, low medium or high below)
medium.
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change. If not, put "none".
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist