nsivabalan commented on PR #19138: URL: https://github.com/apache/hudi/pull/19138#issuecomment-5041275412
Reviewed the config against current apache/master. Overall: low-risk, net-positive, and the config demonstrably works — the Codecov comment on this very PR renders all 12 components, the per-flag table, and carryforward. Approvable once the DNM comment is reverted and one stale path is fixed. **On the disputed flag list:** I independently verified against current master `.github/workflows/bot.yml` — the uploaded flags are exactly the 7 documented: `spark-client-hadoop-common`, `utilities`, `common-and-other-modules`, `spark-java-tests`, `spark-scala-tests`, `hadoop-mr-java-client`, and `integration-tests` (line 1452). There is no `spark-core-tests` flag; the earlier bot comment appears to have checked a stale file. @yihua's rebuttal is correct — no change needed. **Component coverage completeness:** I mapped every top-level module against `component_management` + `ignore`. All second-level subdirs of the multi-module parents (hudi-client, hudi-spark-datasource, hudi-sync, hudi-flink-datasource) are accounted for; `hudi-tests-common` has no coverable main source (log4j resources only). The one module in neither list is `hudi-trino-plugin` — see below. ### Findings 🚨 **Blocking (already acknowledged):** revert the temporary DNM comment in `Base64CodecUtil.java` before merge. ⚠️ **Stale ignore path — `MercifulJsonConverter`:** the kept entry `hudi-common/src/main/java/org/apache/hudi/avro/MercifulJsonConverter.java` no longer resolves; the class moved to `org/apache/hudi/common/avro/MercifulJsonConverter.java`. This contradicts the PR description's note that only still-resolving entries were kept. Either update the path or drop the entry — and since the class now has dedicated tests (`TestMercifulJsonConverter`), dropping it and letting it count may be the better call. 💬 **`hudi-trino-plugin` is in neither `component_management` nor `ignore`.** Currently harmless — its `test-hudi-trino-plugin` job in bot.yml does not upload to Codecov, so nothing reaches the report. But the config nicely documents intent for `hudi-platform-service`; suggest doing the same for Trino (a one-line comment, an ignore, or a component) so a reader can tell the omission is deliberate. 💬 **`notify.wait_for_ci: true` vs path-filtered jobs:** carryforward covers the coverage *number* when a job is skipped, but `wait_for_ci` governs *when* the PR comment fires. This PR touched `hudi-common`, which triggers every job, so it doesn't exercise that path. Worth one experiment with a flink-only or docs-only PR to confirm the comment still posts promptly when several flags never upload. 💅 Nits: `-Djacoco.skip=false` in the README example is redundant (root pom defines no `jacoco.skip` property; the plugin runs by default under `-Punit-tests`); README is missing a trailing newline. ### Suggested validation before merge 1. A throwaway PR touching only `hudi-flink-datasource` (or docs) to confirm skipped flags retain prior coverage via carryforward and the PR comment posts despite `wait_for_ci: true`. 2. After fixing the `MercifulJsonConverter` entry, confirm the file is handled as intended in the next Codecov file list. 3. After reverting the `Base64CodecUtil` comment, confirm the final diff shows zero coverable-line changes. No breaking changes — CI reporting only; the replaced `flags:` block was already dead. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
