Copilot commented on code in PR #11909:
URL: https://github.com/apache/gravitino/pull/11909#discussion_r3527804155
##########
integration-test-common/docker-script/inspect_ip.sh:
##########
@@ -18,5 +18,24 @@
# under the License.
#
+cd "$(dirname "$0")"
-docker inspect --format='{{.Name}}:{{range
.NetworkSettings.Networks}}{{.IPAddress}}{{end}}' $(docker ps -q) |grep
"/trino-ci-" | sed 's/\/trino-ci-//g'
+container_urls=$(docker inspect --format='{{.Name}}:{{range
.NetworkSettings.Networks}}{{.IPAddress}}{{end}}' $(docker ps -q) | grep
"/trino-ci-" | sed 's/\/trino-ci-//g')
Review Comment:
`inspect_ip.sh` inspects **all** running containers on the host (`docker ps
-q`) and then filters by name prefix. With subset support, stale `trino-ci-*`
containers from previous runs (or other jobs on the same host) can leak into
the output and cause flaky/incorrect URL mappings. Prefer sourcing
`compose-env.sh` and using `docker compose -f "$COMPOSE_FILE" ps -q` as the
container ID source, so the script only inspects containers belonging to the
selected compose project/file.
##########
integration-test-common/docker-script/shutdown.sh:
##########
@@ -22,10 +22,14 @@ cd "$(dirname "$0")"
LOG_DIR=../build/trino-ci-container-log
if [ -d $LOG_DIR ]; then
- docker cp trino-ci-hive:/usr/local/hadoop/logs $LOG_DIR/hdfs
- docker cp trino-ci-hive:/tmp/root $LOG_DIR/hive
- docker cp trino-ci-trino:/tmp/trino.log $LOG_DIR/trino.log
+ if [ -n "$(docker ps -aq -f name=trino-ci-hive)" ]; then
+ docker cp trino-ci-hive:/usr/local/hadoop/logs $LOG_DIR/hdfs
+ docker cp trino-ci-hive:/tmp/root $LOG_DIR/hive
+ fi
+ if [ -n "$(docker ps -aq -f name=trino-ci-trino)" ]; then
+ docker cp trino-ci-trino:/tmp/trino.log $LOG_DIR/trino.log
+ fi
fi
export GRAVITINO_TRINO_CONNECTOR_DIR=/dev/null
-docker compose down
+docker compose down --remove-orphans
Review Comment:
`launch.sh` now selects the compose file via `COMPOSE_FILE`, but
`shutdown.sh` always runs `docker compose down` without `-f "$COMPOSE_FILE"`
and does not source `compose-env.sh`. This breaks the new subset-selection
contract (and makes the `TEST_CONTAINERS` env passed from Java ineffective).
Update `shutdown.sh` to source `./compose-env.sh` (after `cd`) and run `docker
compose -f "$COMPOSE_FILE" down --remove-orphans` so shutdown consistently
targets the selected subset.
##########
integration-test-common/docker-script/launch.sh:
##########
@@ -55,12 +60,12 @@ max_attempts=300
attempts=0
while true; do
- docker compose exec -T trino trino --execute "SELECT 1" >/dev/null 2>&1 &&
{
+ docker compose -f "$COMPOSE_FILE" exec -T trino trino --execute "SELECT 1"
>/dev/null 2>&1 && {
break;
}
- num_container=$(docker ps --format '{{.Names}}' | grep trino-ci | wc -l)
- if [ "$num_container" -lt 4 ]; then
+ num_container=$(docker compose -f "$COMPOSE_FILE" ps -q | wc -l)
+ if [ "$num_container" -lt "$EXPECTED_SERVICE_COUNT" ]; then
echo "ERROR: Trino-ci containers start failed."
exit 0
fi
Review Comment:
On container startup failure this exits with status `0`, which signals
success to callers/CI. Change this to a non-zero exit code (e.g., `exit 1`) so
failures are correctly detected and the test run stops with an error.
##########
integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/TrinoITContainers.java:
##########
@@ -106,78 +115,51 @@ private void resolveServerAddress() throws Exception {
String command = ITUtils.joinPath(dockerComposeDir, "inspect_ip.sh");
Object output =
CommandExecutor.executeCommandLocalHost(
- command, false, ProcessData.TypesOfData.STREAMS_MERGED);
+ command, false, ProcessData.TypesOfData.STREAMS_MERGED,
containerEnv);
LOG.info("Command {} output:\n{}", command, output);
// expect the output to be like:
- // trino:10.20.30.21
- // hive:10.20.30.19
- // mysql:10.20.30.20
- // postgresql:10.20.30.18
-
- String containerIpMapping = output.toString();
- if (containerIpMapping.isEmpty()) {
+ // trino_uri=http://10.20.30.21:8080
+ // hive_uri=thrift://10.20.30.19:9083
+ // hdfs_uri=hdfs://10.20.30.19:9000
+ // mysql_uri=jdbc:mysql://10.20.30.20:3306
+ // postgresql_uri=jdbc:postgresql://10.20.30.18
+
+ String serviceURLs = output.toString();
+ if (serviceURLs.isEmpty()) {
throw new ContainerLaunchException(
"Failed to get the container status, the containers have not
started");
}
try {
- String[] containerInfos = containerIpMapping.split("\n");
- for (String container : containerInfos) {
- String[] info = container.split(":");
-
- String containerName = info[0];
- String address = info[1];
-
- if (containerName.equals("trino")) {
- servicesUri.put("trino", String.format("http://%s:8080";, address));
- } else if (containerName.equals("hive")) {
- servicesUri.put("hive_metastore", String.format("thrift://%s:9083",
address));
- servicesUri.put("hdfs", String.format("hdfs://%s:9000", address));
- } else if (containerName.equals("mysql")) {
- servicesUri.put("mysql", String.format("jdbc:mysql://%s:3306",
address));
- } else if (containerName.equals("postgresql")) {
- servicesUri.put("postgresql", String.format("jdbc:postgresql://%s",
address));
+ String[] serviceInfos = serviceURLs.split("\n");
+ for (String serviceInfo : serviceInfos) {
+ String[] info = serviceInfo.split("=", 2);
+ if (info.length != 2) {
+ continue;
}
+
+ servicesUri.put(info[0], info[1]);
}
} catch (Exception e) {
- throw new ContainerLaunchException("Unexpected container status :\n" +
containerIpMapping, e);
+ throw new ContainerLaunchException("Unexpected container status :\n" +
serviceURLs, e);
}
- for (String serviceName : servicesName) {
- if (!servicesUri.containsKey(serviceName)) {
- throw new ContainerLaunchException(
- String.format("The container for the %s service is not started: ",
serviceName));
- }
+ if (!servicesUri.containsKey("trino_uri")) {
+ throw new ContainerLaunchException("The container for the trino service
is not started");
}
}
public void shutdown() {
String command = ITUtils.joinPath(dockerComposeDir, "shutdown.sh");
Object output =
CommandExecutor.executeCommandLocalHost(
- command, false, ProcessData.TypesOfData.STREAMS_MERGED);
+ command, false, ProcessData.TypesOfData.STREAMS_MERGED,
containerEnv);
LOG.info("Command {} output:\n{}", command, output);
}
- public String getTrinoUri() {
- return servicesUri.get("trino");
- }
-
- public String getHiveMetastoreUri() {
- return servicesUri.get("hive_metastore");
- }
-
- public String getHdfsUri() {
- return servicesUri.get("hdfs");
- }
-
- public String getMysqlUri() {
- return servicesUri.get("mysql");
- }
-
- public String getPostgresqlUri() {
- return servicesUri.get("postgresql");
+ public Map<String, String> getServiceUrls() {
+ return servicesUri;
}
Review Comment:
`getServiceUrls()` returns the internal mutable `servicesUri` map directly,
allowing callers to accidentally mutate container state (which can lead to
hard-to-debug test failures). Return an unmodifiable view or a defensive copy
(e.g., `Collections.unmodifiableMap(new HashMap<>(servicesUri))`) to keep the
container object's state encapsulated.
##########
web/integration-test/src/test/java/org/apache/gravitino/integration/test/web/ui/CatalogsPageTest.java:
##########
@@ -123,10 +123,11 @@ public void before() throws Exception {
trinoITContainers = ContainerSuite.getTrinoITContainers();
trinoITContainers.launch(getGravitinoServerPort());
- hiveMetastoreUri = trinoITContainers.getHiveMetastoreUri();
- hdfsUri = trinoITContainers.getHdfsUri();
- mysqlUri = trinoITContainers.getMysqlUri();
- postgresqlUri = trinoITContainers.getPostgresqlUri();
+ Map<String, String> serviceUrls = trinoITContainers.getServiceUrls();
+ hiveMetastoreUri = serviceUrls.get("hive_uri");
+ hdfsUri = serviceUrls.get("hdfs_uri");
+ mysqlUri = serviceUrls.get("mysql_uri");
+ postgresqlUri = serviceUrls.get("postgresql_uri");
Review Comment:
These assignments will silently set fields to `null` if a key is missing
(which is now more likely with container subsets), and later failures will be
less actionable. Consider validating required keys up-front (and failing with a
clear message listing missing keys), or using `getOrDefault(...)` when a
fallback is valid. Also, the repeated string keys (`"hive_uri"`, etc.) are now
duplicated across multiple call sites—centralizing them as constants (e.g., in
`TrinoITContainers`) would reduce drift and typos.
--
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]
