test(flink): improve sink append and partitioner test coverage - #19399
Conversation
4a2c7b8 to
da031aa
Compare
cshuo
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19399 +/- ##
============================================
+ Coverage 72.19% 74.86% +2.66%
+ Complexity 32595 32359 -236
============================================
Files 2561 2574 +13
Lines 148328 142978 -5350
Branches 18654 17719 -935
============================================
- Hits 107087 107037 -50
+ Misses 32774 27886 -4888
+ Partials 8467 8055 -412
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR fills in unit-test coverage for the Flink append-buffer variants, dynamic bucket assignment, record-level-index backend, and BulkInsertWriterHelper, with the only production change being a visibility widening (@VisibleForTesting) on inferMemorySizeForCache. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. A couple of minor readability suggestions on list vs. array assertion and the nested try-finally teardown in the test harness.
cc @yihua
danny0405
left a comment
danny0405
left a comment
There was a problem hiding this comment.
The buffer tests are useful, but the new teardown path still leaves the async-clustering resources open and orders IO-manager shutdown too early.
Address review comments 3670917923 and 3670736056.
Address review comment 3670736052.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR adds focused unit-test coverage for the Flink append buffer variants, dynamic bucket assignment, record-level-index backend, and BulkInsertWriterHelper, with the only production change being a private → package-private @VisibleForTesting widening of RecordLevelIndexBackend.inferMemorySizeForCache(). I traced the new DynamicBucketAssignFunction tests against the production processElement/checkpoint logic and the shared closeAll teardown ordering, and they line up with the actual contracts. No new issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review. A couple of minor test-readability suggestions below, otherwise clean.
cc @yihua
| @ParameterizedTest | ||
| @EnumSource(value = BufferType.class, names = {"CONTINUOUS_SORT", "DISRUPTOR", "BOUNDED_IN_MEMORY"}) | ||
| public void testBufferedAppendFunctionsWriteAndFlushOnCheckpoint(BufferType bufferType) throws Exception { | ||
| Configuration writeConf = TestConfigurations.getDefaultConf(tempFile.getAbsolutePath()); |
There was a problem hiding this comment.
🤖 nit: this writeConf setup block (operation/metadata/buffer type/sort keys/size + the continuous/disruptor if-else) is duplicated verbatim in testBufferedAppendFunctionsPersistRowsInSortOrder — could you pull it into a small buildWriteConf(bufferType) helper?
5 participants
Describe the issue this Pull Request addresses
Closes #19398.
The Flink sink append buffer variants and dynamic bucket assignment paths had substantial unit-test coverage gaps. The existing end-to-end drivers are integration-test-only, so these paths need focused unit tests that run in the regular Maven/Codecov workflow.
Summary and Changelog
name,agesort order.BulkInsertWriterHelperconstructor and error-wrapping paths.InsertFunctionWrapper.Local JaCoCo line coverage from the targeted suite:
AppendWriteFunctionWithContinuousSortAppendWriteFunctionWithDisruptorBufferSortAppendWriteFunctionWithBIMBufferSortDynamicBucketAssignFunctionRecordLevelIndexBackendBulkInsertWriterHelperValidation:
mvn -Punit-tests -pl hudi-flink-datasource/hudi-flink -am \ -Dtest=TestAppendWriteFunction,TestAppendWriteFunctionWithBufferSort,TestAppendWriteFunctionWithBIMBufferSort,TestDynamicBucketAssignFunction,TestRecordLevelIndexBackend,TestBulkInsertWriteHelper \ -Dsurefire.failIfNoSpecifiedTests=false \ -DskipITs -DskipSparkTests -DskipScalaTests testResult: 45 tests run, 0 failures, 0 errors, 0 skipped.
Impact
No public API or user-facing behavior changes. The only production-source adjustment marks the cache-size inference helper package-visible with
@VisibleForTesting; the remaining changes add tests and improve test-harness resource cleanup.Risk Level
Low. The targeted Maven reactor build, Checkstyle, RAT, and JaCoCo report all completed successfully.
Documentation Update
None.
Contributor's checklist