FANNG1 commented on PR #10517: URL: https://github.com/apache/gravitino/pull/10517#issuecomment-4183438679
Thanks for the detailed review. I re-checked these points against the multi-version split spec in #10442 and handled them as follows: - Issue 1: accepted. The unused Iceberg `CatalogFactory.Context` parameter does not belong in the phase-1 mechanical split, so I removed it from the common and 1.18 Iceberg construction path. - Issue 2: accepted. I replaced the direct `AbstractCatalog` casts in the Iceberg common and 1.18 paths with an explicit type check and added a focused unit test. - Issue 3: accepted as a maintainability improvement. I kept `CatalogCompatFlink118` as the explicit 1.18 hook, but made it delegate to `DefaultCatalogCompat` instead of duplicating the same logic. - Issue 4: accepted. I removed the extra `CatalogTableBuilder` indirection from `FlinkGenericTableUtil` and routed the path directly through `CatalogCompat`. - Issue 5: I do not think this is a bug. In this path the outer Flink factory identifier is `gravitino-iceberg`, but the nested native Iceberg factory still requires `catalog-type=iceberg`. That overwrite is the intended bridge between the two layers. I added a code comment to make that explicit instead of changing the behavior. - Issue 6: partially accepted. I added JavaDoc for the new `CatalogStore` hooks/helpers. I did not widen `discoverFactories(...)` to `protected`, because it is still an internal helper rather than an intended versioned extension point. I also addressed the earlier JDBC review comments locally by restoring the pre-split factory option contract, since this PR should stay behavior-preserving per the spec. Validation run locally: - `./gradlew :flink-connector:flink-common:test -PskipITs --tests "org.apache.gravitino.flink.connector.iceberg.TestGravitinoIcebergCatalog" --tests "org.apache.gravitino.flink.connector.hive.TestFlinkGenericTableUtil" --tests "org.apache.gravitino.flink.connector.store.TestGravitinoCatalogStore"` - `./gradlew :flink-connector:flink-1.18:test -PskipITs` I will push the follow-up commits after I finish collecting review feedback on this PR. -- 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]
