Copilot commented on code in PR #4610:
URL: https://github.com/apache/solr/pull/4610#discussion_r3823186864
##########
gradle/testing/randomization/policies/solr-tests.policy:
##########
@@ -276,6 +276,11 @@ grant codeBase "file:${gradle.worker.jar}" {
permission java.security.AllPermission;
};
+// Global override for local integration testing
+grant {
+ permission java.security.AllPermission;
+};
Review Comment:
This unscoped grant gives every test and dependency `AllPermission`, so on
Java versions where `tests.useSecurityManager` is enabled it silently disables
the sandbox for the entire repository. Please remove this global grant and
scope the Testcontainers workaround to `cross-dc-manager` (the existing
Testcontainers setup in `solr/modules/extraction/build.gradle:23-25` disables
the security manager only for that module).
##########
solr/cross-dc-manager/src/test/org/apache/solr/crossdc/manager/KafkaContainerRule.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.solr.crossdc.manager;
+
+import java.lang.invoke.MethodHandles;
+import org.junit.Assume;
+import org.junit.rules.ExternalResource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testcontainers.kafka.KafkaContainer;
+import org.testcontainers.utility.DockerImageName;
+
+/**
+ * JUnit rule that manages a single Kafka broker testcontainer. Declare as a
{@code @ClassRule} so
+ * the (expensive to start) broker is shared across all {@code @Test} methods
in a class instead of
+ * being restarted for each one; JUnit starts it before, and stops it after,
the whole class runs.
+ *
+ * <p>Skips the calling test (via {@link Assume}) instead of failing outright
if
+ * Docker/Testcontainers isn't available in this environment.
+ */
+public class KafkaContainerRule extends ExternalResource {
+
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static final String KAFKA_DOCKER_IMAGE = "apache/kafka:4.3.1";
+
+ private KafkaContainer kafkaContainer;
+
+ @Override
+ protected void before() {
+ kafkaContainer = new
KafkaContainer(DockerImageName.parse(KAFKA_DOCKER_IMAGE));
+ try {
+ kafkaContainer.start();
+ } catch (Throwable t) {
+ Assume.assumeNoException("Docker/Testcontainers not available; skipping
test", t);
+ }
+ }
Review Comment:
Catching every `Throwable` turns Kafka regressions—such as a missing image
tag, incompatible container configuration, linkage errors, or even an OOM—into
a successful test skip. Check Docker availability explicitly, then let failures
from `kafkaContainer.start()` fail the suite so this dependency upgrade is
actually validated.
##########
solr/cross-dc-manager/src/test/org/apache/solr/crossdc/manager/DeleteByQueryToIdTest.java:
##########
@@ -114,7 +113,7 @@ public String bootstrapServers() {
.configure();
props.setProperty("solr.crossdc.topicName", TOPIC);
- props.setProperty("solr.crossdc.bootstrapServers",
kafkaCluster.bootstrapServers());
+ System.setProperty(BOOTSTRAP_SERVERS, bootstrapServers);
Review Comment:
This now writes the bootstrap address to a JVM system property instead of to
`props`, so the `/crossdc.properties` bytes stored immediately below contain
only the topic name. The test therefore no longer exercises the ZooKeeper
bootstrap configuration and succeeds only because the process-wide override
masks the missing entry. Preserve the previous configuration source by setting
this value on `props`.
##########
solr/cross-dc-manager/build.gradle:
##########
@@ -65,6 +64,8 @@ dependencies {
testRuntimeOnly libs.bytebuddy
testRuntimeOnly libs.bytebuddy.agent
+ testImplementation libs.testcontainers.kafka
+
Review Comment:
With Testcontainers replacing `EmbeddedKafkaCluster`, the two Kafka
test-classifier dependency blocks immediately below are obsolete: repository
search finds no remaining imports of Kafka integration/test utilities, and
their own comment says they exist solely for `EmbeddedKafkaCluster`. Remove
those blocks so unused Kafka internal test artifacts are not retained on the
test classpath and in license metadata.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
