[HUDI-5592] Fixing some of the flaky tests in CI #7720
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nsivabalan
force-pushed
the
debug_flaky_tests
branch
from
a17aabe to
744487a
Compare
Contributor
Author
|
@hudi-bot run azure |
nsivabalan
force-pushed
the
debug_flaky_tests
branch
from
744487a to
ed1aa3c
Compare
xushiyan
reviewed
Jan 20, 2023
| // round robin to ensure we generate inserts for all partition paths | ||
| String partitionToUse = partitionPaths[partitionIndex.get()]; | ||
| partitionIndex.set((partitionIndex.get() + 1) % partitionPaths.length); | ||
| return partitionToUse; |
Member
There was a problem hiding this comment.
so with this change we can guarantee all target partitions have records. then we don't need to bump 10 records to 100? so we can make it faster. We just need to make sure num records > num partitions here
Contributor
Author
There was a problem hiding this comment.
yes, but I don't expect any savings from 100 to 10 records. Also most of our tests are doing 100 recs. so just to keep it in parity.
yes, we have only 3 partitions in TestDataGenerator. so, a minimum of 3 is required to ensure we generate for all 3 partitions.
Member
There was a problem hiding this comment.
this is minor: 1) from UT perspective, 100 records shouldn't be different from 10 records. the question is: if 10 works, why bump to 100? 2) we should ensure num records > num partitions without knowing there are always 3 partitions so some check arg will help prevent misuse leading to unexpected results
| tableBasePath = basePath + "/testtable_" + tableType; | ||
| prepareInitialConfigs(fs(), basePath, "foo"); | ||
| TypedProperties props = prepareMultiWriterProps(fs(), basePath, propsFilePath); | ||
| props.setProperty(HoodieCompactionConfig.PARQUET_SMALL_FILE_LIMIT.key(), "0"); |
Member
There was a problem hiding this comment.
should this be applicable to some cases in TestHoodieDeltaStreamer ?
Contributor
Author
There was a problem hiding this comment.
yes, I did not want to boil the ocean just yet. so just focussing on most frequently failing tests. will file a jira to track other similar tests. https://issues.apache.org/jira/browse/HUDI-5595
xushiyan
approved these changes
Jan 21, 2023
Member
xushiyan
left a comment
xushiyan
left a comment
There was a problem hiding this comment.
let's land this without doing another round of CI. we should standardize data generation for testing once for all in some later PRs
nsivabalan
force-pushed
the
debug_flaky_tests
branch
from
ed1aa3c to
ed32d0a
Compare
nsivabalan
force-pushed
the
debug_flaky_tests
branch
from
ed32d0a to
923044d
Compare
This was referenced Jan 24, 2023
alexeykudinkin
added
priority:critical
Collaborator
Contributor
Author
|
CI failed due to flaky tests. |
yihua
pushed a commit
that referenced
this pull request
Jan 30, 2023
Recently we have more flakiness in our CI runs. So, taking a stab at fixing some of the high frequent tests. Tests that are fixed: TestHoodieClientOnMergeOnReadStorage ( testReadingMORTableWithoutBaseFile, testCompactionOnMORTable, testLogCompactionOnMORTable, testLogCompactionOnMORTableWithoutBaseFile) Reasoning for flakiness: we generate only 10 inserts in our tests and it does not guarantee we have records for all 3 partitions(HoodieTestDataGenerator). Fixes: HoodieTestDataGenerator was choosing random partition among list of partitions while generating insert records. Fixed that to do round robin. Also, bumped up the num of records inserted in some of the flaky tests to 100 from 10. Fixed respective MOR tests to disable small file handling.
fengjian428
pushed a commit
to fengjian428/hudi
that referenced
this pull request
Jan 31, 2023
Recently we have more flakiness in our CI runs. So, taking a stab at fixing some of the high frequent tests. Tests that are fixed: TestHoodieClientOnMergeOnReadStorage ( testReadingMORTableWithoutBaseFile, testCompactionOnMORTable, testLogCompactionOnMORTable, testLogCompactionOnMORTableWithoutBaseFile) Reasoning for flakiness: we generate only 10 inserts in our tests and it does not guarantee we have records for all 3 partitions(HoodieTestDataGenerator). Fixes: HoodieTestDataGenerator was choosing random partition among list of partitions while generating insert records. Fixed that to do round robin. Also, bumped up the num of records inserted in some of the flaky tests to 100 from 10. Fixed respective MOR tests to disable small file handling.
yihua
added
priority:blocker
nsivabalan
added a commit
to nsivabalan/hudi
that referenced
this pull request
Mar 22, 2023
Recently we have more flakiness in our CI runs. So, taking a stab at fixing some of the high frequent tests. Tests that are fixed: TestHoodieClientOnMergeOnReadStorage ( testReadingMORTableWithoutBaseFile, testCompactionOnMORTable, testLogCompactionOnMORTable, testLogCompactionOnMORTableWithoutBaseFile) Reasoning for flakiness: we generate only 10 inserts in our tests and it does not guarantee we have records for all 3 partitions(HoodieTestDataGenerator). Fixes: HoodieTestDataGenerator was choosing random partition among list of partitions while generating insert records. Fixed that to do round robin. Also, bumped up the num of records inserted in some of the flaky tests to 100 from 10. Fixed respective MOR tests to disable small file handling.
fengjian428
pushed a commit
to fengjian428/hudi
that referenced
this pull request
Apr 5, 2023
Recently we have more flakiness in our CI runs. So, taking a stab at fixing some of the high frequent tests. Tests that are fixed: TestHoodieClientOnMergeOnReadStorage ( testReadingMORTableWithoutBaseFile, testCompactionOnMORTable, testLogCompactionOnMORTable, testLogCompactionOnMORTableWithoutBaseFile) Reasoning for flakiness: we generate only 10 inserts in our tests and it does not guarantee we have records for all 3 partitions(HoodieTestDataGenerator). Fixes: HoodieTestDataGenerator was choosing random partition among list of partitions while generating insert records. Fixed that to do round robin. Also, bumped up the num of records inserted in some of the flaky tests to 100 from 10. Fixed respective MOR tests to disable small file handling.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Logs
Recently we have more flakiness in our CI runs. So, taking a stab at fixing some of the high frequent tests.
Tests that are fixed:
TestHoodieDeltaStreamerWithMultiWriter.* (all tests)
TestHoodieClientOnMergeOnReadStorage ( testReadingMORTableWithoutBaseFile, testCompactionOnMORTable,
testLogCompactionOnMORTable, testLogCompactionOnMORTableWithoutBaseFile)
Reasoning for flakiness:
Fixes:
Impact
NA
Risk level
None
Documentation Update
NA
Contributor's checklist