FANNG1 commented on PR #12541: URL: https://github.com/apache/gravitino/pull/12541#issuecomment-5694601156
@diqiu50 Thanks for the detailed review. I went through each point and made the following updates or clarifications. 1. `GravitinoDriverPlugin` already preserved user-defined `spark.sql.catalog.<name>` entries before this PR: `registerCatalog()` rejected a collision, and its caller retained the user configuration after logging the failure. The new early return changes the diagnostic level, not the precedence behavior. I do not think a release note is needed. The same path also handles discovery-generated entries, for which WARN would be noisy; distinguishing those sources for logging can be improved separately. 2. Fixed. A catalog name that the default policy passes through but Spark cannot reference unquoted now logs a WARN and is skipped, so it does not abort Spark startup. A custom policy that returns such a name still fails fast. The parser call now goes through `ParserInterface`, allowing the code to catch only `ParseException`. 3. This is a valid test-coverage observation. The current IT proves the supported bundle matrix works, but its provider is defined by the application classloader, so it does not by itself prove the Namespace client is loaded from the bundle. The supported client APIs are compatible across the tested bundle versions, and the provider avoids a static Apache HttpClient method descriptor by using reflection, which is important for the relocated runtime JAR. I consider a stricter published-artifact and classpath-isolation test a follow-up enhancement rather than a blocker for this PR. I will also make the supported bundle-version boundary explicit in the documentation. 4-6. Fixed in `eefa646da`: added discovery and skip diagnostics, improved Lance REST error messages including HTTP status and response body, and made policy/provider loading failures include the relevant configuration and REST endpoint context. 7. Fixed in `eefa646da`: added coverage for runtime validation, policy loading and collisions, listing failures, misspelled configuration keys, and Lance REST error responses. The HTTP-server assertions now run on the test thread. 8. Agreed that the design document was stale. I updated it to state that Lance is the first supported provider. The Lance root list is authenticated but intentionally not per-catalog authorization-filtered: authenticated callers that can access the endpoint can discover its catalog names. This is the discovery contract for this PR, not a prerequisite for support; existing table and namespace authorization remains unchanged. Per-principal catalog-name filtering can be added later if that visibility restriction becomes required. 9. Fixed in `eefa646da`: the user documentation now states that discovery is a startup dependency with no fallback, so an unreachable REST server fails Spark session initialization. Please let me know if you think the root-list visibility contract should instead be treated as a product requirement for 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]
