[HUDI-3178] Fixing metadata table compaction so as to not include uncommitted data#4530
Conversation
nsivabalan
force-pushed
the
metadataCompactionIgnoreFailedCommits
branch
from
33af796 to
f67f3a7
Compare
|
@manojpec : Can you review the patch too. |
nsivabalan
left a comment
nsivabalan
left a comment
There was a problem hiding this comment.
Will go ahead and land this. Let me know if you have any more feedback. Will take it up in a follow up.
| .get().getTimestamp(); | ||
| List<HoodieInstant> pendingInstants = dataMetaClient.reloadActiveTimeline().filterInflightsAndRequested() | ||
| .findInstantsBefore(latestDeltacommitTime).getInstants().collect(Collectors.toList()); | ||
| .findInstantsBefore(instantTime).getInstants().collect(Collectors.toList()); |
There was a problem hiding this comment.
compactionInstantTime at line 703 has to be based off instantTime and not latestDeltaCommitTime. Latest delta commit time is not part of the compaction yet. Otherwise we are changing the meaning of the current compaction timeline with this change.
There was a problem hiding this comment.
let me try to explain.
lets say we have 10 commits, C1, C2 -> C10.
Prior to this patch, we will compact immediately after C10 and so compaction commit will be C10 + "001".
With this patch, we will be compacting just before C11 starts getting applied to MDT.
And so, I am basing the compaction commit of latest delta commit time which is C10 and not instant time which is C11.
And so, its C10 + "001". but if I go with instantTime, then we might change the behavior. In fact, we can't do that, since compaction time will be greater than the delta commit which will be eventually created when we apply C11 to MDT.
Let me know if this makes sense.
There was a problem hiding this comment.
Right, I was actually asking the compaction time to be C10 and not C11. I misread line 689. Look good then.
…ommitted data (#4530) - There is a chance that the actual write eventually failed in data table but commit was successful in Metadata table, and if compaction was triggered in MDT, compaction could have included the uncommitted data. But once compacted, it may never be ignored while reading from metadata table. So, this patch fixes the bug. Metadata table compaction is triggered before applying the commit to metadata table to circumvent this issue.
…ommitted data (apache#4530) - There is a chance that the actual write eventually failed in data table but commit was successful in Metadata table, and if compaction was triggered in MDT, compaction could have included the uncommitted data. But once compacted, it may never be ignored while reading from metadata table. So, this patch fixes the bug. Metadata table compaction is triggered before applying the commit to metadata table to circumvent this issue.
…ommitted data (apache#4530) - There is a chance that the actual write eventually failed in data table but commit was successful in Metadata table, and if compaction was triggered in MDT, compaction could have included the uncommitted data. But once compacted, it may never be ignored while reading from metadata table. So, this patch fixes the bug. Metadata table compaction is triggered before applying the commit to metadata table to circumvent this issue.
…ommitted data (apache#4530) - There is a chance that the actual write eventually failed in data table but commit was successful in Metadata table, and if compaction was triggered in MDT, compaction could have included the uncommitted data. But once compacted, it may never be ignored while reading from metadata table. So, this patch fixes the bug. Metadata table compaction is triggered before applying the commit to metadata table to circumvent this issue.
|
|
||
| try (SparkRDDWriteClient writeClient = new SparkRDDWriteClient(engineContext, metadataWriteConfig, true)) { | ||
| if (canTriggerTableService) { | ||
| // trigger compaction before doing the delta commit. this is to ensure, if this delta commit succeeds in metadata table, but failed in data table, |
There was a problem hiding this comment.
Why is it the case that MT commit could succeed, while Data Table commit could fail?
MT table should only be updated after we're done with the Data Table changes, and right before we complete the txn, right?
5 participants
What is the purpose of the pull request
Brief change log
(for example:)
Verify this pull request
(Please pick either of the following options)
This pull request is a trivial rework / code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Committer checklist
Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.