roryqi commented on code in PR #13569:
URL: https://github.com/apache/gravitino/pull/13569#discussion_r4293671327
##########
iceberg/iceberg-rest-server/build.gradle.kts:
##########
@@ -205,7 +205,12 @@ tasks.test {
// Exclude integration tests
exclude("**/integration/test/**")
} else {
- dependsOn(tasks.jar)
+ dependsOn(
+ tasks.jar,
+ ":catalogs:catalog-lakehouse-iceberg:jar",
+ ":iceberg:iceberg-rest-experimental-server:copyDepends",
+ ":iceberg:iceberg-rest-experimental-server:jar"
+ )
Review Comment:
Fixed in 692a72147a. IcebergExperimentalAuthorizationIT is excluded from the
normal test task and moved to a dedicated experimentalTest task. Ordinary
Iceberg REST integration-test selections retain only the regular jar
dependencies and no longer build or resolve the experimental module.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/IcebergExperimentalAuthorizationIT.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.iceberg.integration.test;
+
+import com.google.common.collect.ImmutableList;
+import java.nio.file.Paths;
+import java.util.HashMap;
+import java.util.UUID;
+import org.apache.gravitino.authorization.Privilege;
+import org.apache.gravitino.authorization.Privileges;
+import org.apache.gravitino.authorization.SecurableObject;
+import org.apache.gravitino.authorization.SecurableObjects;
+import org.apache.gravitino.integration.test.util.TestDatabaseName;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Integration test for Spark authorization through the Iceberg REST service
loaded from the
+ * experimental classpath.
+ *
+ * <p>The inherited tests exercise table creation, loading, updates, deletion,
listing, renaming,
+ * nested namespaces, and table privilege enforcement against the experimental
Iceberg artifacts.
+ */
+@Tag("gravitino-docker-test")
+public class IcebergExperimentalAuthorizationIT extends
IcebergTableAuthorizationIT {
+
+ private static final String EXPERIMENTAL_SERVER_DIRECTORY =
"iceberg-rest-experimental-server";
+
+ @BeforeAll
+ @Override
+ public void startIntegrationTest() throws Exception {
+ String rootDirectory = System.getenv("GRAVITINO_ROOT_DIR");
+ String experimentalServerHome =
+ Paths.get(rootDirectory, "iceberg",
EXPERIMENTAL_SERVER_DIRECTORY).toString();
+ customConfigs.put(
+ "gravitino.iceberg-rest.classpath",
+ String.join(
+ ",",
+ Paths.get(experimentalServerHome, "build", "libs").toString(),
+ Paths.get(experimentalServerHome, "build",
"dependencies").toString()));
Review Comment:
Thanks, fixed. The experimental class now runs only in the dedicated
experimentalTest task, whose runtime classpath substitutes the stable Iceberg
artifacts with the experimental artifacts, so embedded mode no longer silently
exercises stable Iceberg. The test pins IcebergBuild.gitCommitId() and checks
the server startup log; deploy mode additionally verifies that the logged code
source contains iceberg-rest-experimental-server. The 16-test suite passes with
Spark 4.1.3.
##########
build.gradle.kts:
##########
@@ -1168,6 +1168,7 @@ tasks {
"copyJobsLib",
":authorizations:copyLibAndConfig",
":iceberg:iceberg-rest-server:copyLibAndConfigs",
+ ":iceberg:iceberg-rest-experimental-server:copyLibAndConfigs",
Review Comment:
Thanks. We confirmed that the com.datastrato experimental artifacts are a
repackage of Apache Iceberg under Apache License 2.0, rather than a separate
third-party project. The existing Apache Iceberg entry therefore covers them,
and the redundant Datastrato Experimental Iceberg entry was removed in
291c6986f9. The additional upstream NOTICE content required by the packaged
artifacts remains in NOTICE.bin.
##########
iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/IcebergExperimentalAuthorizationIT.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.iceberg.integration.test;
+
+import com.google.common.collect.ImmutableList;
+import java.nio.file.Paths;
+import java.util.HashMap;
+import java.util.UUID;
+import org.apache.gravitino.authorization.Privilege;
+import org.apache.gravitino.authorization.Privileges;
+import org.apache.gravitino.authorization.SecurableObject;
+import org.apache.gravitino.authorization.SecurableObjects;
+import org.apache.gravitino.integration.test.util.TestDatabaseName;
+import org.apache.iceberg.exceptions.ForbiddenException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Integration test for Spark authorization through the Iceberg REST service
loaded from the
+ * experimental classpath.
+ *
+ * <p>The inherited tests exercise table creation, loading, updates, deletion,
listing, renaming,
+ * nested namespaces, and table privilege enforcement against the experimental
Iceberg artifacts.
+ */
+@Tag("gravitino-docker-test")
+public class IcebergExperimentalAuthorizationIT extends
IcebergTableAuthorizationIT {
+
+ private static final String EXPERIMENTAL_SERVER_DIRECTORY =
"iceberg-rest-experimental-server";
+
+ @BeforeAll
+ @Override
+ public void startIntegrationTest() throws Exception {
+ String rootDirectory = System.getenv("GRAVITINO_ROOT_DIR");
+ String experimentalServerHome =
+ Paths.get(rootDirectory, "iceberg",
EXPERIMENTAL_SERVER_DIRECTORY).toString();
+ customConfigs.put(
+ "gravitino.iceberg-rest.classpath",
+ String.join(
+ ",",
+ Paths.get(experimentalServerHome, "build", "libs").toString(),
+ Paths.get(experimentalServerHome, "build",
"dependencies").toString()));
+ super.startIntegrationTest();
+ }
+
+ @Override
+ protected TestDatabaseName getTestDatabaseName() {
+ return TestDatabaseName.PG_ICEBERG_EXPERIMENTAL_AUTHZ_IT;
+ }
+
+ @Test
+ void testSparkCreateNamespaceAuthorization() {
+ String namespace = "experimental_authz_ns";
+ revokeUserRoles();
+ resetMetalakeAndCatalogOwner();
+
+ grantCatalogPrivileges(Privileges.UseCatalog.allow());
+ sql("USE %s", SPARK_CATALOG_NAME);
+
+ Assertions.assertThrowsExactly(
+ ForbiddenException.class, () -> sql("CREATE DATABASE %s", namespace));
+
+ grantCatalogPrivileges(Privileges.CreateSchema.allow());
+ Assertions.assertDoesNotThrow(() -> sql("CREATE DATABASE %s", namespace));
+ Assertions.assertDoesNotThrow(() -> sql("USE %s", namespace));
+ }
Review Comment:
Fixed in 692a72147a. Removed the duplicate namespace authorization test and
its custom privilege helper. The experimental subclass now runs the same
inherited table-authorization suite as the stable configuration, with 16 tests
passing.
##########
iceberg/iceberg-rest-experimental-server/build.gradle.kts:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+description = "iceberg-rest-experimental-server"
+
+plugins {
+ `maven-publish`
+ id("java")
+ id("idea")
+}
+
+val datastratoIcebergVersion: String = libs.versions.datastrato.iceberg.get()
+
+configurations.configureEach {
+ resolutionStrategy.eachDependency {
+ if (
+ requested.name.startsWith("iceberg-") &&
+ (requested.group == "org.apache.iceberg" || requested.group ==
"com.datastrato")
+ ) {
+
useTarget("com.datastrato:experimental-${requested.name}:$datastratoIcebergVersion")
+ because("Use the Datastrato Iceberg distribution in the experimental
auxiliary service")
+ }
+ }
+}
+
+dependencies {
+ implementation(project(":common")) {
+ exclude("*")
+ }
+ implementation(project(":core")) {
+ exclude("*")
+ }
+ implementation(project(":iceberg:iceberg-common")) {
+ exclude("*")
+ }
Review Comment:
Fixed in 692a72147a. Removed the three redundant project dependencies and
the no-source compileJava dependency. The module now expresses its intent with
only iceberg-rest-server plus the Iceberg bundle, and copyDepends handles
assembling the substituted runtime classpath.
##########
docs/iceberg-rest-service.md:
##########
@@ -102,14 +102,27 @@ The server-level
`gravitino.fetchFile.blockUnsafeRemoteUri` configuration contro
#### Auxiliary Service
-| Configuration item | Description
| Default value | Required |
-|------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------|
-| `gravitino.auxService.names` | The auxiliary service name of the
Gravitino Iceberg REST catalog service. Use **`iceberg-rest`**.
| (none) | Yes |
-| `gravitino.iceberg-rest.classpath` | The classpath of the Gravitino Iceberg
REST catalog service; includes the directory containing jars and configuration.
It supports both absolute and relative paths, for example,
`iceberg-rest-server/libs, iceberg-rest-server/conf` | (none) | Yes
|
+| Configuration item | Description
| Default value | Required |
+|------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------|
+| `gravitino.auxService.names` | The auxiliary service name of the
Gravitino Iceberg REST catalog service. Use **`iceberg-rest`**.
| (none) | Yes
|
+| `gravitino.iceberg-rest.classpath` | The classpath of the Gravitino Iceberg
REST catalog service. Use `iceberg-rest-server/libs, iceberg-rest-server/conf`
for the regular Iceberg dependencies or `iceberg-rest-experimental-server/libs,
iceberg-rest-experimental-server/conf` for the experimental dependencies. It
supports both absolute and relative paths. | (none) | Yes |
These settings apply only to `gravitino.conf`.
Do not add them to the standalone server configuration.
+The auxiliary service name and all Iceberg REST configuration keys remain the
same when using the
+experimental dependencies. Only select the experimental classpath:
+
+```properties
+gravitino.auxService.names = iceberg-rest
+gravitino.iceberg-rest.classpath = iceberg-rest-experimental-server/libs,
iceberg-rest-experimental-server/conf
+```
+
+The selected directory is loaded by the Iceberg REST auxiliary service's
isolated classloader. The
+experimental directory uses the Datastrato Iceberg distribution published
under the
+`com.datastrato` Maven group. Its version is pinned by `datastrato-iceberg` in
+`gradle/libs.versions.toml`, independently of the regular Iceberg version.
Review Comment:
Fixed in 692a72147a. The documentation now says the selected directory is
prepended, explains that it takes effect when Iceberg classes are absent from
the base classpath, states that only one regular or experimental directory can
be active, and notes that startup logs expose the effective Iceberg version and
code source.
##########
iceberg/iceberg-rest-experimental-server/build.gradle.kts:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+description = "iceberg-rest-experimental-server"
+
+plugins {
+ `maven-publish`
+ id("java")
+ id("idea")
+}
+
+val datastratoIcebergVersion: String = libs.versions.datastrato.iceberg.get()
+
+configurations.configureEach {
+ resolutionStrategy.eachDependency {
+ if (
+ requested.name.startsWith("iceberg-") &&
+ (requested.group == "org.apache.iceberg" || requested.group ==
"com.datastrato")
+ ) {
+
useTarget("com.datastrato:experimental-${requested.name}:$datastratoIcebergVersion")
+ because("Use the Datastrato Iceberg distribution in the experimental
auxiliary service")
+ }
Review Comment:
Thanks, fixed in 692a72147a. The packaged experimental server runtime forces
the Gravitino Caffeine 2.9.3 and Jackson 2.15.2 versions, and copyDepends is a
Sync task so stale higher-version jars are removed. The dedicated Spark 4.1
test runtime separately allows its required Scala Jackson version while
retaining the packaged-server pins.
--
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]
