[HUDI-5624] Fix HoodieAvroRecordMerger to use new precombine API #7759
Conversation
|
@bschell since this didn't fail any test, can we add/amend existing test validating the regression is fixed |
bschell
force-pushed
the
smallfix
branch
2 times, most recently
from
1b84c1d to
3027c66
Compare
|
@alexeykudinkin I added an end-to-end test for PartialUpdateAvroPayload that catches this regression. While doing so I found and fixed another bug in HoodieAvroRecordMerger here: https://github.com/apache/hudi/blob/master/hudi-common/src/main/java/org/apache/hudi/common/model/HoodieAvroRecordMerger.java#L76 Basically HoodieMetadataPayload is not the only payload class that returns an entirely new payload. This breaks any of the "merge" payloads. I was thinking it might be even better to add this check as an API to HoodieRecordPayload so that we do not have to hardcode all the exceptions here but I kept the smaller change for now as we are deprecating this path later anyway. |
|
|
||
| import scala.collection.JavaConversions._ | ||
|
|
||
| class TestPartialUpdateAvroPayloadE2E extends HoodieClientTestBase { |
There was a problem hiding this comment.
Thanks for adding the test @bschell!
We can drop the E2E suffix for it
| // NOTE: When payload class returns a new payload instead of choosing from older/newer payload, create a new record. | ||
| return new HoodieAvroRecord(newer.getKey(), picked, newer.getOperation()); | ||
| } | ||
| return picked.equals(((HoodieAvroRecord) newer).getData()) ? newer : older; |
There was a problem hiding this comment.
I think we can nuke this optimization avoiding creation of the new record and just always create it in that case (to make sure this doesn't affect all random payloads out there)
bschell
force-pushed
the
smallfix
branch
2 times, most recently
from
5a68b12 to
8bb7953
Compare
Updates the HoodieAvroRecordMerger to use the new precombine API instead of the deprecated one. This fixes issues with backwards compatibility.
|
rebased w/ laster master. we landed a flaky test fix which was causing failures w/ CI runs |
…che#7759) Updates the HoodieAvroRecordMerger to use the new precombine API instead of the deprecated one. This fixes issues with backwards compatibility with certain payloads.
…che#7759) Updates the HoodieAvroRecordMerger to use the new precombine API instead of the deprecated one. This fixes issues with backwards compatibility with certain payloads.
|
I believe it introduced a regression. The new record is always picking |
|
Yes, it is fixed in #8953 |
7 participants
Updates the HoodieAvroRecordMerger to use the new precombine API instead of the deprecated one. This fixes issues with backwards compatibility with certain payloads.
Change Logs
Updates the HoodieAvroRecordMerger to use the new precombine API instead of the deprecated one. This fixes issues with backwards compatibility with certain payloads.
Impact
Fixes bugs
Risk level (write none, low medium or high below)
low
Documentation Update
none
Contributor's checklist