FANNG1 commented on PR #10560: URL: https://github.com/apache/gravitino/pull/10560#issuecomment-4151754044
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]
