jerryshao commented on code in PR #13584:
URL: https://github.com/apache/gravitino/pull/13584#discussion_r4130357181
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/iceberg/GravitinoIcebergCatalog.java:
##########
@@ -74,6 +74,34 @@ public class GravitinoIcebergCatalog extends BaseCatalog
@Override
protected TableCatalog createAndInitSparkCatalog(
String name, CaseInsensitiveStringMap options, Map<String, String>
properties) {
+ String catalogBackendName =
IcebergPropertiesUtils.getCatalogBackendName(properties);
+ Map<String, String> all =
+ buildSparkCatalogProperties(
+ name,
+ options,
+ properties,
+ SparkSession.active().sparkContext().conf(),
+ () -> GravitinoCatalogManager.get().getIcebergRestUri());
+ TableCatalog icebergCatalog = new SparkCatalog();
+ icebergCatalog.initialize(catalogBackendName, new
CaseInsensitiveStringMap(all));
+ return icebergCatalog;
+ }
+
+ Map<String, String> buildSparkCatalogProperties(
+ String name,
+ CaseInsensitiveStringMap options,
+ Map<String, String> properties,
+ SparkConf sparkConf,
+ Supplier<Optional<String>> endpointDiscovery) {
+ Optional<String> icebergRestUri =
+ resolveIcebergRestUri(properties, key -> sparkConf.get(key, null),
endpointDiscovery);
+ if (icebergRestUri.isPresent()) {
Review Comment:
[Question] Moving `resolveIcebergRestUri` ahead of the driver preload also
changes which error a misconfigured catalog reports, which the description's
"no user-facing change" does not cover. For a jdbc-backed catalog whose driver
is absent *and* whose routing cannot resolve an endpoint, the old order threw
`RuntimeException(ClassNotFoundException)` from line 110 first; now
`resolveIcebergRestUri` throws first - either the
missing-`credential-providers` `IllegalStateException` (line 170) or the
no-endpoint / discovery-failure `IllegalStateException` (lines 191 and 202).
That looks like a strict improvement to me: the routing message names the
actual misconfiguration and tells the user how to fall back, whereas the
`ClassNotFoundException` pointed at a driver the routed catalog would never
have used. Worth confirming it is intended, and possibly worth a line in the PR
description so anyone bisecting a changed error message finds it.
Verified by: compared `git diff origin/main...HEAD` statement order against
the current file, and read the three throw sites at
`GravitinoIcebergCatalog.java:170`, `:191` and `:202`.
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/iceberg/GravitinoIcebergCatalog.java:
##########
@@ -74,6 +74,34 @@ public class GravitinoIcebergCatalog extends BaseCatalog
@Override
protected TableCatalog createAndInitSparkCatalog(
String name, CaseInsensitiveStringMap options, Map<String, String>
properties) {
+ String catalogBackendName =
IcebergPropertiesUtils.getCatalogBackendName(properties);
+ Map<String, String> all =
+ buildSparkCatalogProperties(
+ name,
+ options,
+ properties,
+ SparkSession.active().sparkContext().conf(),
+ () -> GravitinoCatalogManager.get().getIcebergRestUri());
+ TableCatalog icebergCatalog = new SparkCatalog();
+ icebergCatalog.initialize(catalogBackendName, new
CaseInsensitiveStringMap(all));
+ return icebergCatalog;
+ }
+
+ Map<String, String> buildSparkCatalogProperties(
+ String name,
+ CaseInsensitiveStringMap options,
+ Map<String, String> properties,
+ SparkConf sparkConf,
+ Supplier<Optional<String>> endpointDiscovery) {
Review Comment:
[Question] This widens a previously private code path into a package-private
method purely so the test can call it, but it carries no marker saying so. The
module already uses Guava's annotation for exactly this
(`GravitinoCatalogManager.java:23` imports
`com.google.common.annotations.VisibleForTesting`, and
`IcebergPropertiesConverter.buildIcebergRestProperties` at
`IcebergPropertiesConverter.java:106` is another package-private test seam).
Adding `@VisibleForTesting` here would make the intent explicit and stop a
future reader from assuming the reduced visibility is load-bearing. Not
blocking either way - just confirming you meant package-private rather than
private.
Verified by: read `GravitinoIcebergCatalog.java` in full at `3696d8c` and
grepped the module for `VisibleForTesting` usage.
##########
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/iceberg/TestGravitinoIcebergCatalogRestRouting.java:
##########
@@ -226,6 +244,42 @@ void
testAutoRoutedRestClientConfigStripsPrefixFromRealSparkConf() {
Assertions.assertEquals("admin", result.get("rest.auth.basic.username"));
}
+ @Test
+ void testRoutedCatalogDoesNotRequireBackendJdbcDriver() {
+ SparkConf sparkConf = new SparkConf(false);
+ sparkConf.set(GravitinoSparkConfig.GRAVITINO_ICEBERG_REST_URI,
"http://manual/iceberg";);
+
+ Map<String, String> result =
+ new GravitinoIcebergCatalog()
+ .buildSparkCatalogProperties(
+ "iceberg_jdbc",
+ CaseInsensitiveStringMap.empty(),
+ jdbcPropertiesWithMissingDriver(),
+ sparkConf,
+ Optional::empty);
+
+ Assertions.assertEquals("http://manual/iceberg";,
result.get(IcebergConstants.URI));
+ }
+
+ @Test
+ void testLegacyCatalogPreloadsBackendJdbcDriver() {
Review Comment:
[Nit] This test stops at the preload throw, so the rest of the legacy branch
is still uncovered. `new GravitinoIcebergCatalog()` never runs `initialize`,
which leaves the inherited `gravitinoCatalogClient` null
(`BaseCatalog.java:96`), and `CredentialPropertyUtils.getCredentials`
dereferences it unconditionally (`CredentialPropertyUtils.java:123-125`). So
`GravitinoIcebergCatalog.java:115-119` - `toSparkCatalogProperties` plus
`applyIcebergCredentials` - would NPE if the driver ever resolved, and no test
in this class reaches it.
That is fine for what this PR fixes, but the new seam is one mocked field
away from covering the whole legacy branch: setting the protected
`gravitinoCatalogClient` to a mocked `Catalog` and using a driver class that
exists (e.g. `org.sqlite.JDBC`, already used in
`TestIcebergCatalogWrapper.java:153`) would let a second case assert that the
legacy path still merges vended credentials.
Verified by: read `BaseCatalog.java:88-105` and
`CredentialPropertyUtils.java:123-130` in this checkout to confirm the null
field and the unguarded dereference.
--
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]
