andygrove opened a new pull request, #2146: URL: https://github.com/apache/datafusion-ballista/pull/2146
# Which issue does this PR close? Closes #2145. # Rationale for this change `docs/source/user-guide/configs.md` was hand-maintained, so it had drifted from the config keys the code actually reads. Nine keys were documented nowhere, and two more only incidentally (in the 54.0.0 upgrade notes and the benchmarking guide). Fixing those gaps by hand would leave the same problem in place. Since `ballista/core/src/config.rs` already builds a runtime registry (`CONFIG_ENTRIES`) carrying each key's name, description, type, and default, the reference tables can be generated from it, and CI can fail when a registered key is documented nowhere. That turns a recurring maintenance chore into a build failure with an actionable message. This follows the approach the sibling DataFusion repo already uses (`dev/update_config_docs.sh` plus a CI check), adapted to Ballista's `ci/scripts/` convention so the failure names the offending file or key rather than printing a raw diff. # What changes are included in this PR? **Generator.** A new binary, `ballista/core/src/bin/update_config_docs.rs`, reads `BallistaConfig::valid_entries()` and rewrites the body of marker-delimited regions in `docs/source/**/*.md`: ``` <!-- BEGIN GENERATED CONFIG REFERENCE prefix=ballista.shuffle. --> <!-- END GENERATED CONFIG REFERENCE --> ``` The optional `prefix=` filter lets one page carry several scoped tables. The binary supports `--check` to report drift without writing, and fails if any registered key is rendered by no region. All logic lives in the binary rather than in `src/`, so none of it becomes public API. It adds no dependencies — `std` only, including the directory walk. **Config registry.** `ConfigEntry` gains public accessors (`name`, `description`, `data_type`, `default_value`) plus `doc_default`, an optional override for defaults that are machine-dependent or unreadable. `ballista.standalone.parallelism` defaults to `available_parallelism()`, which would otherwise render a different number on every machine and fail the check on any runner; it now renders as `number of available CPU cores`. `ballista.testing.chaos_execution.seed` defaults to the empty string and now renders as `(empty)`. Three descriptions too terse to stand alone in a reference table were reworded. **Documentation.** `configs.md` gains three regions whose prefixes partition the whole key space — session settings, shuffle settings, and a new "Testing and Fault Injection" section giving the chaos-monkey keys a home. All 39 registered keys now appear exactly once. The hand-written scheduler CLI table is unchanged and sits outside every region. **Tooling and CI.** `dev/update_config_docs.sh` regenerates; `ci/scripts/rust_config_docs_check.sh` wraps `--check`; a `config-docs-check` job runs it. `CONTRIBUTING.md` documents the workflow. Two notes on the generated output. Each table is preceded by `<!-- prettier-ignore -->`, with a blank line before each `END` marker. Both were verified empirically against prettier 2.7.1 as necessary for the output to be a fixed point. Without the directive, the existing `prettier` job and the new check job would deadlock, each rewriting what the other demands. Separately, markers are recognised only at column 0 and outside fenced code blocks, so marker syntax shown as an example in prose is not mistaken for a live region. # Are there any user-facing changes? Yes, documentation only. The configuration reference now lists eleven previously undocumented or under-documented settings, and the tables are generated rather than hand-maintained. There are no breaking API changes. `ConfigEntry` gains public accessors, which is additive — the struct was previously public but entirely opaque. `docs/source/user-guide/tuning-guide.md` still carries hand-written tables that overlap the generated ones. Their values agree, so there is no contradiction. Converting them to filtered regions is left to a follow-up, which is doc-only now that the multi-file walk exists. One known gap worth calling out for reviewers: `rust.yml`'s path filters do not include `docs/**`, so a PR that *only* hand-edits `configs.md` will not trigger the check job. The main drift vector — editing `config.rs` — is covered, and a notice at the top of the page warns against hand-editing generated regions. Closing the gap properly would need either re-including the file in the path filters, which runs the full Rust matrix on docs typos, or a separate lightweight workflow. Happy to take direction on which is preferred. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
