-
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-8409] Add test to ensure merge mode during upgrade and downgrade #13009
base: master
Are you sure you want to change the base?
Conversation
if (tableConfig.getPayloadClass() != null | ||
&& tableConfig.getPayloadClass().equals(OverwriteWithLatestAvroPayload.class.getName())) { | ||
if (HoodieTableType.COPY_ON_WRITE == tableConfig.getTableType()) { | ||
if (tableConfig.getPayloadClass() != 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.
Could we add unit tests on these logic in TestSevenToEightUpgradeHandler
besides functional tests? Also, simplify the functional tests because those are time-consuming.
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.
sg. We have some unit tests for the overall upgradehandler logic. let me add some unit tests.
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.
The unit test is added.
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. I am also thinking if we really need a functional test?
why can't we add tests directly against upgrade handler. validate the table config props before and after upgrade and be done with it.
0de0635
to
7348332
Compare
(10, "5", "rider-E", "driver-E", 17.85, "i"), | ||
(10, "3", "rider-C", "driver-C", 33.9, "i"), | ||
(20, "6", "rider-Z", "driver-Z", 27.7, "i")) | ||
val expectedCommitTimeBased: Seq[(Int, String, String, String, Double, String)] = Seq( |
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.
Will make commit time and event time results different.
if (tableConfig.getPayloadClass() != null | ||
&& tableConfig.getPayloadClass().equals(OverwriteWithLatestAvroPayload.class.getName())) { | ||
if (HoodieTableType.COPY_ON_WRITE == tableConfig.getTableType()) { | ||
if (tableConfig.getPayloadClass() != 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.
yes. I am also thinking if we really need a functional test?
why can't we add tests directly against upgrade handler. validate the table config props before and after upgrade and be done with it.
if (HoodieTableType.COPY_ON_WRITE == tableConfig.getTableType()) { | ||
if (tableConfig.getPayloadClass() != null) { | ||
if (tableConfig.getPayloadClass().equals(OverwriteWithLatestAvroPayload.class.getName())) { | ||
if (HoodieTableType.COPY_ON_WRITE == tableConfig.getTableType()) { |
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.
lets first align on all diff combinations and whats expected behavior during upgrade and downgrade before going ahead w/ the patch. we can sync up f2f on this
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.
Blocked on my review and revision
Change Logs
Impact
Clean and clear verification on merge mode during table version change.
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