This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 6693442650 [#13044][#13045] improvement(test): identify the server an
integration test starts (#13048)
6693442650 is described below
commit 669344265093a05043dbd7269954cfe0765799aa
Author: Qi Yu <[email protected]>
AuthorDate: Fri Sep 11 17:37:28 2026 +0800
[#13044][#13045] improvement(test): identify the server an integration test
starts (#13048)
### What changes were proposed in this pull request?
Two changes that let an integration suite know which server it is
talking to.
- `BaseIT` refuses to launch the deploy mode server when something
already listens on the configured port, and names the port in the
message. `ITUtils.checkServerPortIsFree` does the check.
- `MiniGravitinoContext` carries the server to start, defaulting to
`GravitinoServer::main`, and `MiniGravitino` starts that one. Existing
callers are unaffected.
### Why are the changes needed?
**Any server on the port is accepted as ours.** `BaseIT.startServer()`
decides the deploy mode server is ready by polling `/metrics` until an
HTTP server answers, and never checks that the process answering is the
one it just launched. A Gravitino left behind by an interrupted run
satisfies the probe, so the suite runs against a stranger with unrelated
configuration.
This is easy to hit, because `gravitino.sh` fails to launch whenever a
port it needs is taken, including the debugger port, and the leftover
holding that port is usually also holding the server port. In my case
the launch failed four times:
```
ERROR: transport error 202: bind failed: Address already in use
ERROR: JDWP Transport dt_socket failed to initialize, TRANSPORT_INIT(510)
```
and the suite then ran against a leftover server that had authorization
enabled, so `createMetalake` came back as:
```
ForbiddenException: Forbidden error :User 'anonymous' is not authorized to
perform
operation 'createMetalake' : Only service admins can create metalakes
```
even though the configuration under test had
`gravitino.authorization.enable = false` and
`gravitino.authorization.serviceAdmins = anonymous`. The failure points
at authorization, and the cause is a stale process; it took several
rounds to find that in `logs/gravitino-server.out`.
**Embedded mode cannot start a downstream server.** `MiniGravitino`
hardcodes `GravitinoServer.main`, so a distribution that ships its own
entry point can only run integration tests in deploy mode. The failure
mode is quiet rather than loud: in embedded mode the components its
server would initialize are simply absent, so the event listeners that
depend on them throw on every event and the framework logs and swallows
it:
```
WARN ...Listener - Failed to handle event RegisterJobTemplateEvent@201b3159
java.lang.NullPointerException: ... because "this.searchService" is null
```
The suite keeps running, nothing is ever written, and assertions that
tolerate an empty result pass. A test can report success while
exercising nothing.
Fix: #13044
Fix: #13045
### Does this PR introduce _any_ user-facing change?
No. Both changes are in the integration test harness.
For test authors: deploy mode now fails fast, with the port named and an
`lsof` invocation to find the holder, instead of running against
whatever answers. `MiniGravitinoContext` gains a constructor that takes
a `ServerLauncher`; the existing constructor keeps starting
`GravitinoServer`.
### How was this patch tested?
- `TestITUtils` covers both sides of the port check: a free port passes,
and a port held by an open `ServerSocket` raises `IllegalStateException`
whose message names the port.
- `TestMiniGravitino` covers that the default context launches
`GravitinoServer` and that a custom launcher is the one invoked.
- `./gradlew :integration-test-common:test -PskipITs`
Both tests were written first and failed to compile against the old API.
Worth a reviewer's opinion: `EventListenerPluginWrapper` logs and
swallows every listener failure, which is what turned the second problem
into a silent one. A listener failing on every event is closer to a
broken deployment than to a recoverable error. I left that alone here,
but it seems worth addressing separately.
---
.../test/catalog/GravitinoCatalogManagerIT.java | 5 +-
.../gravitino/integration/test/MiniGravitino.java | 10 ++--
.../integration/test/MiniGravitinoContext.java | 57 ++++++++++++++++++++++
.../integration/test/TestMiniGravitino.java | 24 +++++++++
.../gravitino/integration/test/util/BaseIT.java | 6 +++
.../gravitino/integration/test/util/ITUtils.java | 33 +++++++++++++
.../integration/test/util/TestITUtils.java | 30 ++++++++++++
7 files changed, 158 insertions(+), 7 deletions(-)
diff --git
a/flink-connector/flink-common/src/test/java/org/apache/gravitino/flink/connector/integration/test/catalog/GravitinoCatalogManagerIT.java
b/flink-connector/flink-common/src/test/java/org/apache/gravitino/flink/connector/integration/test/catalog/GravitinoCatalogManagerIT.java
index 1792e926dc..3869e6d004 100644
---
a/flink-connector/flink-common/src/test/java/org/apache/gravitino/flink/connector/integration/test/catalog/GravitinoCatalogManagerIT.java
+++
b/flink-connector/flink-common/src/test/java/org/apache/gravitino/flink/connector/integration/test/catalog/GravitinoCatalogManagerIT.java
@@ -64,8 +64,7 @@ public abstract class GravitinoCatalogManagerIT extends
BaseIT {
@BeforeAll
void startUp() throws Exception {
- // Start Gravitino server
- super.startIntegrationTest();
+ // JUnit starts the server through BaseIT before this method.
initGravitinoEnv();
initMetalake();
initFlinkEnv();
@@ -75,7 +74,7 @@ public abstract class GravitinoCatalogManagerIT extends
BaseIT {
@AfterAll
void stop() throws Exception {
stopFlinkEnv();
- super.stopIntegrationTest();
+ // JUnit stops the server through BaseIT after this method.
LOG.info("Stop Flink env successfully.");
}
diff --git
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitino.java
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitino.java
index 65fb60a9b5..841785548b 100644
---
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitino.java
+++
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitino.java
@@ -198,10 +198,12 @@ public class MiniGravitino {
executor.submit(
() -> {
try {
- GravitinoServer.main(
- new String[] {
- ITUtils.joinPath(mockConfDir.getAbsolutePath(),
"gravitino.conf")
- });
+ context
+ .serverLauncher()
+ .launch(
+ new String[] {
+ ITUtils.joinPath(mockConfDir.getAbsolutePath(),
"gravitino.conf")
+ });
} catch (Exception e) {
LOG.error("Exception in startup MiniGravitino Server ", e);
throw new RuntimeException(e);
diff --git
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitinoContext.java
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitinoContext.java
index cf02bb3f3c..e925d14637 100644
---
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitinoContext.java
+++
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/MiniGravitinoContext.java
@@ -20,18 +20,75 @@
package org.apache.gravitino.integration.test;
import java.util.Map;
+import org.apache.gravitino.server.GravitinoServer;
public class MiniGravitinoContext {
+
+ /**
+ * Starts a Gravitino server from a configuration file, in the current JVM.
+ *
+ * <p>A distribution that ships its own entry point initializes components
the open source server
+ * knows nothing about. Without a way to name that entry point, its
integration tests can only run
+ * in deploy mode, and in embedded mode those components are quietly absent:
the event listeners
+ * that depend on them fail on every event, the framework logs and swallows
the failures, and
+ * tests that tolerate empty results pass while exercising nothing.
+ */
+ @FunctionalInterface
+ public interface ServerLauncher {
+
+ /**
+ * Starts the server.
+ *
+ * @param args The arguments to start the server with, the configuration
file path.
+ * @throws Exception If the server fails to start.
+ */
+ void launch(String[] args) throws Exception;
+ }
+
+ /** Starts the open source {@link GravitinoServer}, which is what most
callers want. */
+ public static final ServerLauncher DEFAULT_SERVER_LAUNCHER =
GravitinoServer::main;
+
Map<String, String> customConfig;
final boolean ignoreIcebergAuxRestService;
final boolean ignoreLanceAuxRestService;
+ private final ServerLauncher serverLauncher;
public MiniGravitinoContext(
Map<String, String> customConfig,
boolean ignoreIcebergAuxRestService,
boolean ignoreLanceAuxRestService) {
+ this(
+ customConfig,
+ ignoreIcebergAuxRestService,
+ ignoreLanceAuxRestService,
+ DEFAULT_SERVER_LAUNCHER);
+ }
+
+ /**
+ * Creates a context that starts the given server rather than the open
source one.
+ *
+ * @param customConfig The configuration entries to write into the server
configuration.
+ * @param ignoreIcebergAuxRestService Whether to leave the Iceberg REST
service out.
+ * @param ignoreLanceAuxRestService Whether to leave the Lance REST service
out.
+ * @param serverLauncher The server to start.
+ */
+ public MiniGravitinoContext(
+ Map<String, String> customConfig,
+ boolean ignoreIcebergAuxRestService,
+ boolean ignoreLanceAuxRestService,
+ ServerLauncher serverLauncher) {
this.customConfig = customConfig;
this.ignoreIcebergAuxRestService = ignoreIcebergAuxRestService;
this.ignoreLanceAuxRestService = ignoreLanceAuxRestService;
+ this.serverLauncher = serverLauncher;
+ }
+
+ /**
+ * Returns the server this context starts.
+ *
+ * @return the server launcher.
+ */
+ public ServerLauncher serverLauncher() {
+ return serverLauncher;
}
}
diff --git
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/TestMiniGravitino.java
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/TestMiniGravitino.java
index e0f75347eb..40b915c78d 100644
---
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/TestMiniGravitino.java
+++
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/TestMiniGravitino.java
@@ -30,7 +30,9 @@ import static org.mockito.Mockito.when;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.util.ArrayList;
import java.util.Collections;
+import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.TimeUnit;
import org.apache.gravitino.client.RESTClient;
@@ -41,6 +43,28 @@ class TestMiniGravitino {
@TempDir private Path mockConfDir;
+ @Test
+ void testContextLaunchesTheOpenSourceServerByDefault() {
+ // Existing callers pass no launcher and must keep getting the open source
server.
+ MiniGravitinoContext context = new
MiniGravitinoContext(Collections.emptyMap(), false, false);
+
+ assertSame(MiniGravitinoContext.DEFAULT_SERVER_LAUNCHER,
context.serverLauncher());
+ }
+
+ @Test
+ void testContextCarriesACustomServerLauncher() throws Exception {
+ // A distribution that ships its own entry point initializes components
the open source server
+ // knows nothing about, so embedded mode has to be able to start that
server instead.
+ List<String[]> launched = new ArrayList<>();
+ MiniGravitinoContext context =
+ new MiniGravitinoContext(Collections.emptyMap(), false, false,
launched::add);
+
+ context.serverLauncher().launch(new String[] {"gravitino.conf"});
+
+ assertEquals(1, launched.size());
+ assertEquals("gravitino.conf", launched.get(0)[0]);
+ }
+
@Test
void testStopCleansResourcesWhenServerTaskDoesNotTerminate() throws
Exception {
ExecutorService executor = mock(ExecutorService.class);
diff --git
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/BaseIT.java
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/BaseIT.java
index a2bae427b2..bdbeaba005 100644
---
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/BaseIT.java
+++
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/BaseIT.java
@@ -412,6 +412,12 @@ public class BaseIT {
setupJdbcDrivers();
+ JettyServerConfig configuredJetty =
+ JettyServerConfig.fromConfig(serverConfig, WEBSERVER_CONF_PREFIX);
+ // The readiness check below only asks whether an HTTP server answers,
so a Gravitino left
+ // behind by an earlier run would pass it and the suite would run
against that process.
+ ITUtils.checkServerPortIsFree(configuredJetty.getHost(),
configuredJetty.getHttpPort());
+
GravitinoITUtils.startGravitinoServer();
JettyServerConfig jettyServerConfig =
diff --git
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/ITUtils.java
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/ITUtils.java
index 70862e4de8..67829c3bbb 100644
---
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/ITUtils.java
+++
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/ITUtils.java
@@ -24,6 +24,8 @@ import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
+import java.net.InetSocketAddress;
+import java.net.Socket;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Paths;
@@ -50,6 +52,8 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class ITUtils {
+
+ private static final int PORT_CHECK_TIMEOUT_MS = 500;
private static final Logger LOG = LoggerFactory.getLogger(ITUtils.class);
private static final String CI_ENV = "CI";
private static final String GITHUB_ACTIONS_ENV = "GITHUB_ACTIONS";
@@ -258,4 +262,33 @@ public class ITUtils {
}
private ITUtils() {}
+
+ /**
+ * Refuses to start a server on a port something else already holds.
+ *
+ * <p>The readiness probe that follows a deploy mode launch only asks
whether an HTTP server
+ * answers on the port, not whether it is the one just launched. A Gravitino
left behind by an
+ * interrupted run therefore satisfies it, and the suite proceeds against a
stranger with
+ * unrelated configuration, failing later in ways that point away from the
cause. Checking here
+ * turns that into an immediate, accurate error.
+ *
+ * @param host The host the server is configured to bind.
+ * @param port The port the server is configured to bind.
+ * @throws IllegalStateException If something already listens on the port.
+ */
+ public static void checkServerPortIsFree(String host, int port) {
+ try (Socket socket = new Socket()) {
+ socket.connect(new InetSocketAddress(host, port), PORT_CHECK_TIMEOUT_MS);
+ } catch (IOException e) {
+ // Nothing answered, which is what we want.
+ return;
+ }
+
+ throw new IllegalStateException(
+ String.format(
+ "Something already listens on %s:%d, so the server under test
cannot bind it and the "
+ + "readiness probe would accept the existing process as if it
were ours. Stop it "
+ + "first, for instance with `lsof -nP -iTCP:%d -sTCP:LISTEN`.",
+ host, port, port));
+ }
}
diff --git
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/TestITUtils.java
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/TestITUtils.java
index cb07597989..6744a9109b 100644
---
a/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/TestITUtils.java
+++
b/integration-test-common/src/test/java/org/apache/gravitino/integration/test/util/TestITUtils.java
@@ -18,6 +18,8 @@
*/
package org.apache.gravitino.integration.test.util;
+import java.io.IOException;
+import java.net.ServerSocket;
import java.util.Map;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
@@ -43,4 +45,32 @@ public class TestITUtils {
Assertions.assertFalse(ITUtils.isCiEnvironment(Map.of()));
Assertions.assertFalse(ITUtils.isCiEnvironment(Map.of("CI", "false")));
}
+
+ @Test
+ void testCheckServerPortIsFreeAcceptsAnUnusedPort() throws IOException {
+ int port;
+ try (ServerSocket socket = new ServerSocket(0)) {
+ port = socket.getLocalPort();
+ }
+
+ Assertions.assertDoesNotThrow(() ->
ITUtils.checkServerPortIsFree("localhost", port));
+ }
+
+ @Test
+ void testCheckServerPortIsFreeRejectsAPortSomethingElseHolds() throws
IOException {
+ // A server left behind by an earlier run keeps answering on this port.
Starting a suite against
+ // it runs the tests against a stranger's configuration, so refuse before
the launch rather than
+ // after a readiness probe the leftover satisfies.
+ try (ServerSocket socket = new ServerSocket(0)) {
+ int port = socket.getLocalPort();
+
+ IllegalStateException e =
+ Assertions.assertThrows(
+ IllegalStateException.class, () ->
ITUtils.checkServerPortIsFree("localhost", port));
+
+ Assertions.assertTrue(
+ e.getMessage().contains(String.valueOf(port)),
+ "the message has to name the port so the cause is actionable: " +
e.getMessage());
+ }
+ }
}
