-
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-5822] Fix FileId not found exception when FileId is passed to HoodieMergeHa... #7997
Conversation
hudi-common/src/main/java/org/apache/hudi/common/table/view/TableFileSystemView.java
Outdated
Show resolved
Hide resolved
...tasource/hudi-flink/src/main/java/org/apache/hudi/sink/bucket/BucketStreamWriteFunction.java
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/common/model/HoodieFileGroup.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/common/model/HoodieFileGroup.java
Outdated
Show resolved
Hide resolved
@voonhous, could you please add the test cases for the changes? |
Hmmm, not easy to write a test for this. This bug can only be triggered if JM's rollback is running behind TM. |
@hbgstc123 @TengHuo for visibility |
@voonhous, could you add the unit tests for |
@SteNicholas @danny0405 Can you please help to review this commit? Thank you |
@hudi-bot run azure |
hudi-common/src/test/java/org/apache/hudi/common/testutils/FileCreateUtils.java
Outdated
Show resolved
Hide resolved
@@ -115,16 +115,24 @@ protected HoodieRollbackRequest getRollbackRequestForAppend(String markerFilePat | |||
// TODO(HUDI-1517) use provided marker-file's path instead | |||
Option<HoodieLogFile> latestLogFileOption = FSUtils.getLatestLogFile(table.getMetaClient().getFs(), partitionPath, fileId, | |||
HoodieFileFormat.HOODIE_LOG.getFileExtension(), baseCommitTime); | |||
|
|||
// Log file can be deleted if the commit to rollback is also the commit that created the fileGroup |
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.
@danny0405 here
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.
Okay, seems here is the code you wanna folk from the listing based rollback strategy:
// In case all data was inserts and the commit failed, delete the file belonging to that commit
// We do not know fileIds for inserts (first inserts are either log files or base files),
// delete all files for the corresponding failed commit, if present (same as COW)
hoodieRollbackRequests.add(getHoodieRollbackRequest(partitionPath, filesToDelete));
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.
Yes.
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.
Can we split this PR into 2, one is for the marker based rollback fix, another is for the testing of flink stream writer with hashing index enabled? I mean we create two JIRA issues here.
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.
Sure, good idea!
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.
separate PR: #8077
Close because it should be fixed in #8077 |
@danny0405 This is not fixed... The rollback standardises the rollback logic between markers and listing for MOR tables. The bug is still present in COW tables... TM might run ahead of of JM. If we continue to use |
How could this happen? The write tasks on JM would wait for the JM coordinator to finish initialization then start to hande the writiing process. |
JM
TM
ExplanationTM's Hope this log paints a clearer picture. |
@danny0405 Running the tests included with this PR without the fix fails... Hence, I believe the fix in this PR is still required. |
Got ya, can you rebase with the latest master and force-push again |
Rebased and force-pushed. |
@hudi-bot run azure |
1 similar comment
@hudi-bot run azure |
@voonhous The CI can not be triggered, can you fire another PR instead. |
...ndle
Applying the fix from #5185 will fix write issues for MOR tables, but will cause write issues for COW tables.
More information on how to reproduce the COW error in this jira issue:
https://issues.apache.org/jira/browse/HUDI-5822
This addresses the issue raised here: #5782
Change Logs
getLatestFileSlices
->getAllFileGroups
in [HUDI-3758] Fix duplicate fileId error in MOR table type with flink bucket hash Index #5185Impact
Risk level (write none, low medium or high below)
None
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist