RohanExploit opened a new pull request, #11881: URL: https://github.com/apache/seatunnel/pull/11881
### Purpose of this pull request Part of #11007. I claimed `connector-file` on the umbrella. Two file-connector sinks re-checked, at runtime, options their own factory already declares required in `optionRule()`: - `HdfsFileSinkFactory.initHadoopConf()` ran `CheckConfigUtil.checkAllExists(FS_DEFAULT_NAME_KEY)`, but the same factory's `optionRule()` already declares `required(DEFAULT_FS)`, and `FileBaseOptions.DEFAULT_FS` is keyed on `FS_DEFAULT_NAME_KEY`. - `S3FileSink`'s constructor ran `checkAllExists(FILE_PATH, S3_BUCKET)`, and `S3FileSinkFactory.optionRule()` already declares both as `required`. In both cases the imperative check could only fire after declarative validation had already passed, so it was unreachable duplication that reported the same failure later and in a different format. This removes both and lets `optionRule()` own the validation, so the failure surfaces at `--check` time. Net effect is 36 deleted lines of production code and no new production code. ### One case deliberately left alone `HdfsFileHadoopConfig.buildWithConfig()` also calls `checkAllExists`, on `FILE_PATH`, `FILE_FORMAT_TYPE` and `DEFAULT_FS`. I did **not** touch it, because unlike the two above it is not redundant: - `HdfsFileSourceFactory.optionRule()` declares `DEFAULT_FS` and `FILE_FORMAT_TYPE` as `.optional(...)`, not `.required(...)`. - It pairs `FILE_PATH` with `TABLE_CONFIGS` via `.exclusive(...)`, whereas the imperative check requires `FILE_PATH` unconditionally. - `HdfsFileCatalogFactory.optionRule()` returns `OptionRule.builder().build()`, i.e. empty, and it reaches `buildWithConfig` too. So removing that one would drop validation rather than move it, and reconciling it means deciding whether those options are genuinely required for the HDFS source and catalog. That is a semantic change, and the guide warns against silently converting optional into required, so I left it for a maintainer to weigh in on. Happy to do it in a follow-up if you tell me the intended semantics. ### Does this PR introduce _any_ user-facing change? No behavioral change for valid configurations. For the two invalid cases the error now comes from declarative validation at `--check` time instead of an equivalent `FileConnectorException` raised slightly later during sink construction. Both paths already failed the job. ### How was this patch tested? Regression tests added to the existing factory tests, asserting the declarative rules reject exactly the configurations the removed checks used to reject: - `HdfsFileFactoryTest.sinkOptionRuleRequiresDefaultFs` - `HdfsFileFactoryTest.sinkOptionRuleRequiresFilePath` - `S3FileFactoryTest.sinkOptionRuleRequiresFilePath` - `S3FileFactoryTest.sinkOptionRuleRequiresBucket` Each asserts `OptionValidationException` when the option is absent and no throw once it is supplied. I confirmed the tests are load-bearing rather than decorative by temporarily weakening `.required(S3FileSinkOptions.S3_BUCKET)` to `.optional(...)`, which fails `sinkOptionRuleRequiresBucket` with "Expected OptionValidationException to be thrown, but nothing was thrown", then reverting. I also checked no existing test depended on the removed `FileConnectorException`. Full module suites pass locally on JDK 11: `connector-file-s3` 21 tests, `connector-file-hadoop` 13 tests (2 pre-existing Windows-only skips), and `spotless:check` is clean on both. --- Disclosure: this change was AI-assisted (Claude). I reviewed it and can speak to the reasoning. -- 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]
