nzw921rx commented on PR #11122: URL: https://github.com/apache/seatunnel/pull/11122#issuecomment-4758356623
> Thanks for working on this. I went through the full diff and traced the actual source / sink / catalog creation paths on the current `dev` base. > > The source-side and sink-side migration to declarative `OptionRule + Conditions` is moving in the right direction. In both paths the new rules are on the real hot path: > > ```text > Source creation > -> FactoryUtil.createAndPrepareSource(...) [seatunnel-api/.../FactoryUtil.java:184-193] > -> ConfigValidator.validate(sourceFactory.optionRule()) [L191] > -> ElasticsearchSourceFactory.createSource(...) > -> new ElasticsearchSource(...) > > Sink creation > -> FactoryUtil.createAndPrepareSink(...) [seatunnel-api/.../FactoryUtil.java:210-260] > -> ConfigValidator.validate(tableSinkFactory.optionRule()) [L252] > -> ElasticsearchSinkFactory.createSink(...) > ``` > > I did find one blocker before this can merge: > > 1. `ElasticSearchCatalogFactory.optionRule()` is not on the real catalog creation path today. > - Real path: > > ```text > Catalog resolution > -> CatalogTableUtil.getCatalogTables(...) [seatunnel-api/.../CatalogTableUtil.java:101-142] > -> FactoryUtil.createOptionalCatalog(...) [seatunnel-api/.../FactoryUtil.java:287-295] > -> optionalFactory.map(catalogFactory -> catalogFactory.createCatalog(...)) > ``` > > - Unlike the source/sink paths, `FactoryUtil.createOptionalCatalog(...)` does **not** call `ConfigValidator.of(options).validate(catalogFactory.optionRule())` before `createCatalog(...)`. > - Because of that, the new rules in `connector-elasticsearch/.../ElasticSearchCatalogFactory.java:58-90` are effectively dead on the current `dev` base. They only run in the new unit test, where the test invokes `ConfigValidator` manually. > - That means the PR description currently overstates the result for catalogs: invalid Elasticsearch catalog configs are still not rejected on the real submission path; they can still fail later in `catalog.open()` / `catalog.getTables(...)`. > > The factory tests themselves look deterministic and non-flaky, but they do not cover this real catalog runtime path, so they miss the gap above. > > My merge conclusion for the current head is: **fix before merge**. > > What I would suggest: > - Option A: include the missing catalog validation in `FactoryUtil.createOptionalCatalog(...)`, so the catalog path matches the source/sink validation model end-to-end. > - Option B: if you want to keep this PR scoped to Elasticsearch only, then please drop the catalog migration/tests from this PR for now and land them after the shared catalog validation path is available. > > Happy to re-review once you push an update. > @DanielLeens thanks your review ,PR #11127 Fixed -- 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]
