davidzollo commented on PR #10678: URL: https://github.com/apache/seatunnel/pull/10678#issuecomment-5283321265
@knight6236 I hear the principle you're arguing from, and I don't think it's an unreasonable instinct in the abstract. But I checked it against how this project actually handles comparable off-by-default, explicitly-experimental switches, and I don't think the premise holds up here — so let me push back with evidence rather than just repeating the ask. **The project's actual convention is the opposite of "undocumented = safer."** Two existing docs pages cover features that are exactly this shape — off by default, explicitly experimental/risky, not yet fully hardened: - `docs/en/engines/zeta/engine-jar-storage-mode.md` documents `connector-jar-storage-enable` (default `false`) behind a `:::caution warn` admonition: *"Please note that this feature is currently in an experimental stage, and there are many areas that still need improvement. Therefore, we recommend exercising caution when using this feature to avoid potential issues and unnecessary risks."* - `docs/en/engines/zeta/rest-api-v1.md` documents a deprecated, disabled-by-default API surface with the same `:::caution warn` pattern, and still tells users exactly which config to flip to turn it on. In both cases the project's answer to "this is risky and not production-hardened yet" was to document it clearly, with an explicit caution box, rather than omit it. That's precisely the "experimental caveat" pattern I proposed for `SEATUNNEL_CLASSLOADER_DEEP_CLEAN`. If leaving something out of the docs were actually the safer choice, these two pages would follow the same logic you're applying here — they don't, and I don't think that's an oversight; a caution-boxed doc page reads as "opt in only if you understand the tradeoff," which is a stronger signal than silence. **There's also an inconsistency in the "only source-readers will find it" argument.** The WARN log at `DefaultClassLoaderService.java` already tells an operator, at the exact moment they've hit a partial failure, to add two specific `--add-opens` JVM flags to make deep-clean work more completely. That log line is not a passive caveat — it's active, step-by-step guidance toward enabling more of the feature, delivered with none of the surrounding context (Phase 1/2 status, the JDK 8 global `useCaches` tradeoff, the "subject to change before Phase 3" framing) that a docs paragraph would carry. If the concern is "don't invite premature adoption," the log message you've already shipped is arguably a bigger invitation than a docs page would be, just a worse-informed one. To be clear about scope: I'm not asking for a production usage guide or a recommendation to use this. I'm asking for the same shape of disclosure the project already uses elsewhere for switches in this exact state — one caution-boxed paragraph: property name, default `false`, both `--add-opens` flags, and the "experimental, subject to change before Phase 3" caveat you already wrote out in your comment above. That's a documentation-only change with no code risk. I'll leave the actual merge call to a maintainer with write access — my read-level review can't gate this either way, and this is now a genuine values disagreement rather than a technical defect. But I wanted to make sure the "documenting increases risk" premise was checked against the codebase's own precedent before we settle on leaving it deferred, since on the evidence above I don't think it does. -- 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]
