Copilot commented on code in PR #11219:
URL: https://github.com/apache/gravitino/pull/11219#discussion_r3322624461
##########
flink-connector/v1.18/flink/src/test/java/org/apache/gravitino/flink/connector/integration/test/iceberg/FlinkIcebergRestCatalogIT118.java:
##########
@@ -19,4 +19,9 @@
package org.apache.gravitino.flink.connector.integration.test.iceberg;
+import org.junit.jupiter.api.condition.DisabledIf;
+
+// Flink 1.18 uses Iceberg 1.9.x; lakehouse-iceberg (1.11) runs in embedded
MiniGravitino in the
+// same JVM. Run REST-catalog Iceberg IT in deploy mode only.
+@DisabledIf("org.apache.gravitino.integration.test.util.ITUtils#isEmbedded")
public class FlinkIcebergRestCatalogIT118 extends FlinkIcebergRestCatalogIT {}
Review Comment:
The base class `FlinkIcebergRestCatalogIT` is already annotated with
`@DisabledIf("org.apache.gravitino.integration.test.util.ITUtils#isEmbedded")`
(see `flink-connector/flink-common/.../FlinkIcebergRestCatalogIT.java:43`), and
JUnit 5 `@DisabledIf` is inherited from the superclass. Re-applying the same
annotation on `FlinkIcebergRestCatalogIT118` / `FlinkIcebergRestCatalogIT119`
is redundant and risks looking like an intentional override later. Consider
removing the duplicate annotation here (and on `FlinkIcebergRestCatalogIT119`);
the Hive subclasses do need it since `FlinkIcebergHiveCatalogIT` is not
disabled at the base level.
##########
flink-connector/v1.19/flink/src/test/java/org/apache/gravitino/flink/connector/integration/test/iceberg/FlinkIcebergRestCatalogIT119.java:
##########
@@ -19,4 +19,9 @@
package org.apache.gravitino.flink.connector.integration.test.iceberg;
+import org.junit.jupiter.api.condition.DisabledIf;
+
+// Flink 1.19 uses Iceberg 1.10.x; lakehouse-iceberg (1.11) runs in embedded
MiniGravitino in the
+// same JVM. Run REST-catalog Iceberg IT in deploy mode only.
+@DisabledIf("org.apache.gravitino.integration.test.util.ITUtils#isEmbedded")
public class FlinkIcebergRestCatalogIT119 extends FlinkIcebergRestCatalogIT {}
Review Comment:
Same redundancy as in `FlinkIcebergRestCatalogIT118`: the base
`FlinkIcebergRestCatalogIT` is already disabled in embedded mode at the class
level, so re-applying `@DisabledIf` on this subclass has no effect. Consider
removing it (the Hive subclass `FlinkIcebergHiveCatalogIT119` still needs it).
--
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]
