jerryshao commented on code in PR #13569: URL: https://github.com/apache/gravitino/pull/13569#discussion_r4282765297
########## iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/IcebergExperimentalAuthorizationIT.java: ########## @@ -0,0 +1,64 @@ +/* + * 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 java.nio.file.Paths; +import org.apache.gravitino.integration.test.util.TestDatabaseName; +import org.apache.iceberg.IcebergBuild; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Tag; + +/** + * 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 { + Assertions.assertEquals( + System.getProperty("datastratoIcebergVersion"), + IcebergBuild.version(), + "The experimental integration test must run with the Datastrato Iceberg runtime"); Review Comment: [Important] This guard is a version-string comparison, not a distribution check, so it stops guarding the moment Apache Iceberg publishes 1.13.0. `IcebergBuild.version()` returns the `git.build.version` entry of the first `/iceberg-build.properties` on the classpath. In the fork that value is literally `1.13.0`, so the assertion is `"1.13.0".equals("1.13.0")` -- it distinguishes the fork from stable Iceberg only because `libs.versions.toml:70` currently pins `iceberg = '1.11.0'`. Bump that pin to 1.13.0 (upstream has not released it yet -- `org.apache.iceberg:iceberg-core:1.13.0` is a 404 on Central today, so this is a near-future bump) and the assertion passes against the stable jars, which is exactly the silent-fallback case this class was added to catch. The fork ships a value that cannot collide: `iceberg-build.properties` in `experimental-iceberg-core-1.13.0.jar` has `git.commit.id=e6118db078e09d960600c3c302e0f220457cb388` and `git.branch=codex/read-restrictions-no-ci` (its `git.closest.tag.name` is `apache-iceberg-1.12.0-rc2`, so the `1.13.0` version string is a publishing number, not an upstream release). Asserting `IcebergBuild.gitCommitId()`, or that `IcebergBuild.class.getProtectionDomain().getCodeSource().getLocation()` contains `experimental-iceberg`, would keep working across version bumps. If you prefer the version string, pinning the expected commit alongside it in `libs.versions.toml` is enough. Verified by: extracting `iceberg-build.properties` from `experimental-iceberg-core-1.13.0.jar` and `IcebergBuild.class` from `experimental-iceberg-api-1.13.0.jar` (both fetched from Maven Central in this run), `javap` of `IcebergBuild.loadBuildInfo`, and the pins in `gradle/libs.versions.toml:70-71` on this checkout. ########## iceberg/iceberg-rest-server/build.gradle.kts: ########## @@ -27,8 +27,44 @@ plugins { val scalaVersion: String = project.properties["scalaVersion"] as? String ?: extra["defaultScalaVersion"].toString() val sparkVersion: String = libs.versions.spark34.get() val sparkMajorVersion: String = sparkVersion.substringBeforeLast(".") +val experimentalSparkVersion: String = libs.versions.spark35.get() +val experimentalSparkMajorVersion: String = experimentalSparkVersion.substringBeforeLast(".") val icebergVersion: String = libs.versions.iceberg.get() +val datastratoIcebergVersion: String = libs.versions.datastrato.iceberg.get() val scalaCollectionCompatVersion: String = libs.versions.scala.collection.compat.get() +val caffeineVersion: String = libs.versions.caffeine.get() +val jacksonVersion: String = libs.versions.jackson.get() + +val experimentalTestRuntimeClasspath by configurations.creating { + extendsFrom(configurations.testRuntimeClasspath.get()) + resolutionStrategy { + force( + "com.github.ben-manes.caffeine:caffeine:$caffeineVersion", + "com.fasterxml.jackson.core:jackson-annotations:$jacksonVersion", + "com.fasterxml.jackson.core:jackson-core:$jacksonVersion", + "com.fasterxml.jackson.core:jackson-databind:$jacksonVersion", + "com.fasterxml.jackson.datatype:jackson-datatype-jdk8:$jacksonVersion", + "com.fasterxml.jackson.datatype:jackson-datatype-jsr310:$jacksonVersion" + ) + eachDependency { + if (requested.group == "org.apache.spark") { + useVersion(experimentalSparkVersion) + because("The experimental Iceberg Spark runtime is published for Spark 3.5") Review Comment: [Nit] This changes two variables at once, which weakens the comparison the new task is meant to provide: `experimentalTest` runs Spark 3.5.3 while `test` runs Spark 3.4.3 (`libs.versions.toml:80-81`), on top of the Iceberg distribution swap. A failure that shows up only in `experimentalTest` cannot be attributed to the fork without first ruling out the Spark bump -- and the `assertThrowsExactly` relaxation in `IcebergTableAuthorizationIT` in this same commit is already an instance of that. Nothing to fix in the build if it is unavoidable: `com.datastrato:experimental-iceberg-spark-runtime-3.4_2.12:1.13.0` is a 404 on Central while `...-3.5_2.12` and `...-3.5_2.13` both resolve, so Spark 3.5 is the only option the fork publishes. Worth a comment here saying so, and a line in the PR description, so the next person reading a divergence between the two suites knows the Spark version differs by necessity. Verified by: `curl` of the 3.4_2.12, 3.5_2.12 and 3.5_2.13 POMs on Maven Central in this run (404, 200, 200), and `gradle/libs.versions.toml:80-81` on this checkout. ########## iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/IcebergExperimentalAuthorizationIT.java: ########## @@ -0,0 +1,64 @@ +/* + * 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 java.nio.file.Paths; +import org.apache.gravitino.integration.test.util.TestDatabaseName; +import org.apache.iceberg.IcebergBuild; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Tag; + +/** + * 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 { + Assertions.assertEquals( + System.getProperty("datastratoIcebergVersion"), + IcebergBuild.version(), + "The experimental integration test must run with the Datastrato Iceberg runtime"); + + 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: [Question] Is this classpath override meant to be exercised at all in embedded mode? The assertion above validates the Gradle substitution on the test JVM, not that the server honoured these directories, so the mechanism this PR exists to add is covered only by the deploy matrix job. In embedded mode the server runs inside the test JVM, and `experimentalTestRuntimeClasspath` has already put the fork's Iceberg there. `IsolatedClassLoader.isSharedClass` is `!isCatalogClass(name)` for `org.apache.iceberg.*` (`core/src/main/java/org/apache/gravitino/utils/IsolatedClassLoader.java:242`, `268-273`), so the aux service resolves Iceberg from the base classloader and never consults these two directories. The assertion passes either way. In deploy mode it is different and genuinely covered: `BaseIT.rewriteGravitinoServerConfig` writes `customConfigs` into the deployed `conf/gravitino.conf` (`integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/BaseIT.java:177-194`), and `copySubprojectDependencies` keeps `iceberg*` modules out of the server's own `libs` (`build.gradle.kts:1480`), so the directory really is what the service loads from. Now that `RESTService.serviceInit` logs `IcebergBuild.fullVersion()` and the code source, a server-side check is cheap: assert the integration-test log (`gravitino.log.path`, set per module in `build.gradle.kts:479`) contains `experimental-iceberg` after startup. That would make the embedded run fail rather than pass when the override is inert, and would cover the deploy run too. Verified by: reading `IsolatedClassLoader` (lines 230-300), `BaseIT.rewriteGravitinoServerConfig`, and the distribution filter in the root build file, all on this checkout. ########## iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/integration/test/IcebergTableAuthorizationIT.java: ########## @@ -272,7 +272,7 @@ void testLoadTable() { setSchemaOwner(NORMAL_USER); Assertions.assertDoesNotThrow(() -> sql("DESC TABLE %s", tableName)); // With ownership: non-existent table returns 404 (AnalysisException) - Assertions.assertThrowsExactly( + Assertions.assertThrows( Review Comment: [Nit] This relaxes an assertion in the *stable* suite to accommodate the experimental one. `assertThrows` accepts any `AnalysisException` subclass, so the stable Spark 3.4.3 run no longer pins the exact type it used to, for a reason that only exists in the Spark 3.5.3 subclass run. Keeping the exact check where it still holds and widening it only for the subclass -- a `protected` hook the experimental class overrides, or asserting on the message instead of the exact type -- would preserve the stable coverage. Low stakes either way; flagging it because the weakening is invisible from the stable suite's side. Verified by: reading lines 262-282 of this file on this checkout, and the `org.apache.spark` version substitution added in `iceberg/iceberg-rest-server/build.gradle.kts:50-52` in the same commit, which is the only reason this line changed in this PR. ########## iceberg/iceberg-rest-server/build.gradle.kts: ########## @@ -27,8 +27,44 @@ plugins { val scalaVersion: String = project.properties["scalaVersion"] as? String ?: extra["defaultScalaVersion"].toString() val sparkVersion: String = libs.versions.spark34.get() val sparkMajorVersion: String = sparkVersion.substringBeforeLast(".") +val experimentalSparkVersion: String = libs.versions.spark35.get() +val experimentalSparkMajorVersion: String = experimentalSparkVersion.substringBeforeLast(".") val icebergVersion: String = libs.versions.iceberg.get() +val datastratoIcebergVersion: String = libs.versions.datastrato.iceberg.get() val scalaCollectionCompatVersion: String = libs.versions.scala.collection.compat.get() +val caffeineVersion: String = libs.versions.caffeine.get() +val jacksonVersion: String = libs.versions.jackson.get() + +val experimentalTestRuntimeClasspath by configurations.creating { + extendsFrom(configurations.testRuntimeClasspath.get()) Review Comment: [Question] `configurations.creating` inherits `testRuntimeClasspath`'s dependencies but none of its attributes -- did you check the resolved jar set matches what `test` gets? `extendsFrom` copies dependencies only. The java plugin sets `Usage=java-runtime`, `LibraryElements=jar`, `Bundling=external` and `TargetJvmVersion` on `testRuntimeClasspath`; this configuration requests nothing, so variant selection falls back to the legacy path (the producer's `default` configuration for project dependencies, and whichever variant a module's Gradle metadata leaves as default). For modules that publish environment-specific variants -- Guava's `jre`/`android` split is the usual one on this classpath -- that can land on a different jar than the stable suite uses, which would make an `experimentalTest`-only failure hard to attribute to the Iceberg fork. `configurations.testRuntimeClasspath.get().copyRecursive()` (then apply the `resolutionStrategy` to the copy), or copying the attributes explicitly, removes the question. I did not run Gradle in this review, so this may already resolve identically -- if you have a `dependencies --configuration experimentalTestRuntimeClasspath` output that matches `testRuntimeClasspath` apart from the intended substitutions, that settles it. Verified by: reading this block on this checkout against the java plugin's attributes on `testRuntimeClasspath`; not reproduced with a Gradle run. ########## LICENSE.bin: ########## @@ -212,6 +212,7 @@ Apache DataSketches Java Apache DataSketches Memory Apache Iceberg + Datastrato Experimental Apache Iceberg Review Comment: [Nit] The entry names a non-ASF artifact as "Apache Iceberg". The published artifact calls itself `Experimental Iceberg` (the `<name>` element of `experimental-iceberg-core-1.13.0.pom`, `groupId com.datastrato`, `url https://github.com/datastrato/iceberg`), so `Datastrato Experimental Iceberg (com.datastrato)` would both match the upstream name and avoid implying the jar is an ASF release -- which matters more here than usual, since this file ships inside the convenience binary. Verified by: `curl` of `experimental-iceberg-core-1.13.0.pom` from Maven Central in this run, read against `LICENSE.bin:209-217` on this checkout. -- 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]
