FANNG1 commented on PR #10560: URL: https://github.com/apache/gravitino/pull/10560#issuecomment-4151753207
I think this feature should be redesigned around the user-facing concept of "session catalog", instead of mixing the new behavior directly into the existing `GravitinoCatalogStore`. My suggestions: 1. The configuration should be modeled as `supportsSessionCatalog` (or an equivalent session-catalog-oriented option), instead of exposing the implementation detail `allow.third-party-connector.list`. If type-level control is still needed, that can be an additional config, but the primary user-facing concept should be "support session catalog", not "route some third-party connectors to memory". 2. Do not mix the new logic into `GravitinoCatalogStore.java`. If `supportsSessionCatalog` is enabled, use a new `GravitinoSessionCatalogStore`; otherwise keep using the original `GravitinoCatalogStore`. This feature introduces a different catalog lifecycle model, so it should have a separate implementation boundary rather than adding more branching into the existing class. 3. The documentation should explicitly define the behavior of session catalogs. Please document the semantics of create/get-use/drop/show-list, the lifecycle of session catalogs, restart behavior, and the precedence/conflict rule when the same catalog name exists in both places. 4. Please add integration tests to cover the full session catalog lifecycle. Unit tests are not enough for this feature. We should have IT coverage for all major operations on session catalogs, including at least create, get/use, show/list, and drop. -- 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]
