rangareddy commented on issue #17398: URL: https://github.com/apache/hudi/issues/17398#issuecomment-5351245430
This issue was reviewed as part of the JIRA-migrated backlog triage (HUDI-9153). **Findings: still open. The fragility described in [#12943 (comment)](https://github.com/apache/hudi/pull/12943#discussion_r1987183961) is unchanged on `master`.** The load-bearing comment is still in the source, and the config declarations below it still cannot be reordered: ```java // hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCleanConfig.java:74 // The cleaner policy config definition has to be before the following configs for inference: // CLEANER_COMMITS_RETAINED, CLEANER_HOURS_RETAINED, CLEANER_FILE_VERSIONS_RETAINED ``` Why it is fragile, in `HoodieConfig`: ```java // hudi-common/src/main/java/org/apache/hudi/common/config/HoodieConfig.java protected void setDefaults(String configClassName) { Arrays.stream(configClass.getDeclaredFields()) // <- iteration order ... .forEach(f -> { ... setDefaultValue(cfgProp); }); // <- mutates props as it goes } public <T> void setDefaultValue(ConfigProperty<T> configProperty) { if (!contains(configProperty)) { ... inferValue = configProperty.getInferFunction().get().apply(this); // sees props mutated so far ``` `setDefaultValue` writes each resolved default straight into `props`, and `CLEANER_POLICY`'s infer function decides via `cfg.contains("hoodie.clean.commits.retained")` etc. So once `CLEANER_COMMITS_RETAINED`'s default of `10` has been materialised, the infer function can no longer distinguish "the user set it" from "we just defaulted it", and the policy inference silently changes. Correctness depends entirely on `CLEANER_POLICY` being visited first. The sharp edge is that `Class#getDeclaredFields()` explicitly does not guarantee order: *"The elements in the returned array are not sorted and are not in any particular order."* Source order happens to be preserved by mainstream JVMs today, but that is unspecified behaviour, so this is a latent correctness bug and not only a style concern. Any refactor, reordering, or code-generation/instrumentation tool that perturbs field order can flip a user's cleaning policy without any test failing. Possible directions: separate default materialisation from inference (run all infer functions against the user-supplied props snapshot rather than the progressively mutated one), or track user-set keys explicitly so `contains()` in an infer function means "explicitly configured" rather than "present in props". A regression test that shuffles the declared-field order and asserts identical resolved configs would lock the behaviour down. Note: `HoodieCleanConfig` is currently the only class carrying such an ordering comment, but the underlying mechanism affects every `ConfigProperty` that combines `withInferFunction` with `contains()` checks, so the audit should be repo-wide. Keeping this open. -- 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]
