-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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-8518] Fix RLI and Secondary index with custom payload or merge mode #12525
Conversation
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
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.
Reviewed source code. will review tests once feedback is addressed.
...di-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java
Outdated
Show resolved
Hide resolved
...di-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
...di-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java
Outdated
Show resolved
Hide resolved
...di-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
6fd129b
to
4cc33ae
Compare
String fileId = writeStatsByFileIdEntry.getKey(); | ||
List<HoodieWriteStat> writeStats = writeStatsByFileIdEntry.getValue(); | ||
String partition = writeStats.get(0).getPartitionPath(); | ||
FileSlice previousFileSliceForFileId = fsView.getLatestFileSlice(partition, fileId).orElse(null); |
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.
We are still using filesystem view for now, instead of populating file slice in the writestats (through append handle). There could be some issue due to NBCC with CDC enabled. Trying to clarify it in #12582
Btw, here fsView call is destributed by partition, fileId. So, not making repeated calls.
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.
Need to review tests. done w/ source code
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadata.java
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
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.
I would like to see more tests for the core method finding SI records.
d3809bc
to
4a07a54
Compare
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.
I still don't see the tests that I asked for.
esply around preparing SI records for MDT given a file slice.
Cases I would like to see:
-
adding 1 new log file w/ data block. Existing file slice has just base file.
a. data block has updates. some of them updates the SI value, while some do not.
b. data block contains some deletes (custom payload way)
c. log file is a delete block. -
Same as 1, just that the existing file slice contains base file and 2 log files.
-
Same as 1. just that existing file slice contains base file, 1 log file w/ data block. and a delete block.
-
Same as 1. existing file slice contains base file and 1 committed log block. one another log file which is partially failed. SI record generation should ignore the partially committed log file.
-
update a SI record in logs(lf1). and later delete it(lf2) and then re-add (lf3) across diff commits. SI record generation should hold water.
-
Simulate, async compaction.
last but one file slice contains base file and 3 log files. latest file slice contains 1 log file. and new commit is looking to add a new log file. SI record generation should work.
May be, you have already covered w/ testSecondaryIndexRecordGenerationForMOR.
can you please confirm.
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java
Outdated
Show resolved
Hide resolved
…erge modes [HUDI-8741] Ensure deletes via custom payload or merger gets synced to RLI [HUDI-8740] Fix SI update logic to avoid collection on the driver
4a07a54
to
e015b4d
Compare
e015b4d
to
a86ece2
Compare
hudi-common/src/main/java/org/apache/hudi/metadata/SecondaryIndexRecordGenerationUtils.java
Show resolved
Hide resolved
@nsivabalan Yes, most of those tests are already covered in |
Change Logs
Impact
Index correctness with custom mergers, and optimization.
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