test(flink): improve streamer config and schema provider coverage - #19392
Conversation
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 unit-test coverage for several previously under-tested hudi-flink classes — streamer/clustering/compaction config parsing, OptionsInference/OptionsResolver resolution paths, and the file-based and schema-registry providers. I traced the non-obvious assertions (runtime-config version gating, scheduler-type resolution, and the write-buffer sizing math) against the production code and they correctly encode current behavior. 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 few nits on test method names and a confusing unused mock variable below.
cc @yihua
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19392 +/- ##
============================================
+ Coverage 74.83% 75.08% +0.25%
- Complexity 32332 32377 +45
============================================
Files 2574 2574
Lines 142978 142985 +7
Branches 17527 17528 +1
============================================
+ Hits 106992 107366 +374
+ Misses 27921 27548 -373
- Partials 8065 8071 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
cshuo
left a comment
cshuo
left a comment
There was a problem hiding this comment.
A few test coverage issues to consider.
5dbd4a1 to
e73a5f9
Compare
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR mostly adds unit-test coverage for the Flink streamer/clustering/compaction config classes and schema providers, and it carries two small production fixes: converting --instant-retry-interval from seconds to milliseconds, and having StreamerUtil.getProps honor --hoodie-conf when no --props file is given. Both production changes look like genuine bug fixes, though they do alter runtime behavior despite the "no runtime impact" note — one worth double-checking is flagged inline. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. A couple of vague test method names worth tightening up.
github-actions
Bot
added
size:XL
| return new TypedProperties(); | ||
| TypedProperties properties = new TypedProperties(); | ||
| cfg.configs.forEach(x -> { | ||
| String[] kv = x.split("="); |
There was a problem hiding this comment.
[P1] Split --hoodie-conf only on the first =. String.split("=") rejects valid values that contain another = (for example a URL/query string or JSON expression), and because Java drops trailing empty fields it also rejects an intentionally empty value such as hoodie.datasource.write.partitionpath.field=. The --props path parses both cases successfully, so the behavior now depends on whether a props file was supplied. Please use split("=", 2) (and add coverage for embedded/empty values) so the no-file path preserves the complete value.
…9392) Adapted for release-1.2.1: TestOptionsResolver#testConcurrencyControlModes keeps only the NON_BLOCKING_CONCURRENCY_CONTROL assertion. The OPTIMISTIC_CONCURRENCY_CONTROL half calls OptionsResolver#isOptimisticConcurrencyControl, which this branch does not have; master re-added that method in 348e7f1 (#18946, "feat(flink): Add validation to reject multiple writers for flink RLI writes"), which is not backported. A comment in the test records the omission. The conflict also offered a HoodieTableConfig import that #19392 does not add and nothing here uses, so it was left out to avoid an unused import. Everything else applies unchanged, including both production changes: the instantRetryInterval seconds-to-millis conversion in FlinkStreamerConfig and the --config key=value parsing in StreamerUtil. (cherry picked from commit ed7327b)
…9392) Adapted for release-1.2.1: TestOptionsResolver#testConcurrencyControlModes keeps only the NON_BLOCKING_CONCURRENCY_CONTROL assertion. The OPTIMISTIC_CONCURRENCY_CONTROL half calls OptionsResolver#isOptimisticConcurrencyControl, which this branch does not have; master re-added that method in 348e7f1 (#18946, "feat(flink): Add validation to reject multiple writers for flink RLI writes"), which is not backported. A comment in the test records the omission. The conflict also offered a HoodieTableConfig import that #19392 does not add and nothing here uses, so it was left out to avoid an unused import. Everything else applies unchanged, including both production changes: the instantRetryInterval seconds-to-millis conversion in FlinkStreamerConfig and the --config key=value parsing in StreamerUtil. (cherry picked from commit ed7327b)
6 participants
Describe the issue this Pull Request addresses
Several unit-testable configuration, streamer wiring, option resolution, and schema provider classes in
hudi-flinkhad low or no line coverage. This left option parsing and validation, derived configuration, schema loading, and schema registry behavior largely unverified outside integration tests.Summary and Changelog
--hoodie-confoverrides when no--propsfile is supplied, and convert the documented retry interval from seconds to milliseconds in the derived Flink configuration.Codecov line coverage before and after from the
common-and-other-modulesFlink unit-test upload on head
fad501fc66fe:FlinkStreamerConfigFlinkClusteringConfigFlinkCompactionConfigHoodieFlinkStreamerOptionsInferenceOptionsResolverSchemaRegistryProviderFilebasedSchemaProviderNo code was copied.
Impact
No public API, configuration default, or performance impact. Runtime configuration now preserves
--hoodie-confoverrides without requiring--props, and interprets--instant-retry-intervalin the documented seconds before populating the millisecond Flink option. The remaining changes improve the Flink unit-test Codecov upload.Risk Level
low. The two runtime changes align behavior with the existing CLI contract and are covered by focused configuration tests plus the full
hudi-flinkunit suite.Documentation Update
none
Contributor's checklist
Testing
hudi-flinkunit suite: 1,384 tests, 0 failures, 0 errors, 1 skipped.