Copilot commented on code in PR #10046: URL: https://github.com/apache/gravitino/pull/10046#discussion_r2875894966
########## integration-test-common/doris-docker-script/start-be.sh: ########## @@ -0,0 +1,87 @@ +#!/bin/bash +# +# 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. +# + +# BE-only startup script for docker-compose multi-container deployment. +# This overrides the image's default start.sh which starts both FE and BE. + +DORIS_HOME="$(dirname "${BASH_SOURCE-$0}")" +DORIS_HOME="$(cd "${DORIS_HOME}" >/dev/null; pwd)" +DORIS_BE_HOME="${DORIS_HOME}/be/" + +# Patch BE startup script: skip vm.max_map_count and ulimit checks, remove slow chmod +DORIS_BE_SCRIPT="${DORIS_BE_HOME}/bin/start_be.sh" +sed -i '/Please set vm.max_map_count/,/exit 1/{s/exit 1/#exit 1\n echo "skip this"/}' ${DORIS_BE_SCRIPT} +sed -i '/Please set the maximum number of open file descriptors/,/exit 1/{s/exit 1/#exit 1\n echo "skip this"/}' ${DORIS_BE_SCRIPT} +sed -i 's/chmod 755 "${DORIS_HOME}\/lib\/doris_be"/#&/' ${DORIS_BE_SCRIPT} + +# Configure priority_networks and report interval in be.conf +CONTAINER_IP=$(hostname -i) +PRIORITY_NETWORKS=$(echo "${CONTAINER_IP}" | awk -F '.' '{print$1"."$2"."$3".0/24"}') +echo "add priority_networks = ${PRIORITY_NETWORKS} to be.conf" +echo "priority_networks = ${PRIORITY_NETWORKS}" >> ${DORIS_BE_HOME}/conf/be.conf +echo "report_disk_state_interval_seconds = 10" >> ${DORIS_BE_HOME}/conf/be.conf + +# Start only BE in daemon mode +${DORIS_BE_HOME}/bin/start_be.sh --daemon + +# Wait for BE to be ready +be_started=false +for i in {1..20}; do + sleep 5 + url="localhost:8040/api/health" + echo "check be for the $i times" + response=$(curl --silent --request GET $url) + + if echo "$response" | grep -q '"status": "OK"'; then + be_started=true + echo "Doris BE started successfully, response: $response" + break + else + echo "check failed, response: $response" + fi +done + +if [ "$be_started" = false ]; then + echo "ERROR: Doris BE failed to start" Review Comment: If BE fails the readiness check (`be_started` is false), the script only prints an error and then proceeds, keeping the container alive. Please `exit 1` (non-zero) on this failure so the compose stack fails fast instead of leaving a broken BE container running. ```suggestion echo "ERROR: Doris BE failed to start" exit 1 ``` ########## integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/DorisContainer.java: ########## @@ -63,78 +84,130 @@ protected DorisContainer( Map<String, String> envVars, Optional<Network> network) { super(image, hostName, ports, extraHosts, filesToMount, envVars, network); - } - @Override - protected void setupContainer() { - super.setupContainer(); - withLogConsumer(new PrintingContainerLog(format("%-14s| ", "DorisContainer"))); - withStartupTimeout(Duration.ofMinutes(5)); + String dir = System.getenv("GRAVITINO_ROOT_DIR"); + if (dir == null || dir.isEmpty()) { + throw new RuntimeException("GRAVITINO_ROOT_DIR is not set"); + } + String composeFile = + ITUtils.joinPath( + dir, "integration-test-common", "doris-docker-script", "docker-compose.yaml"); + + composeContainer = + new ComposeContainer(new File(composeFile)) + .withEnv("GRAVITINO_CI_DORIS_DOCKER_IMAGE", DEFAULT_IMAGE) + .withExposedService( + FE_SERVICE, + FE_MYSQL_PORT, + Wait.forListeningPort().withStartupTimeout(Duration.ofMinutes(3))) + .withExposedService(FE_SERVICE, FE_HTTP_PORT) + .withScaledService(BE_SERVICE, BE_SCALE) + .withStartupTimeout(Duration.ofMinutes(5)) + .withTailChildContainers(true) + .withLocalCompose(true); + + // Expose ports for each BE instance (1-indexed) + for (int i = 1; i <= BE_SCALE; i++) { + composeContainer + .withExposedService( + BE_SERVICE, + i, + BE_HEARTBEAT_PORT, + Wait.forListeningPort().withStartupTimeout(Duration.ofMinutes(3))) + .withExposedService(BE_SERVICE, i, BE_WEBSERVER_PORT); + } } @Override public void start() { - super.start(); + composeContainer.start(); + resolveFeIpAddress(); Preconditions.check("Doris container startup failed!", checkContainerStatus(5)); Preconditions.check("Doris container password change failed!", changePassword()); } @Override public void close() { copyDorisLog(); - super.close(); + if (composeContainer != null) { + composeContainer.stop(); + } } private void copyDorisLog() { try { - // stop Doris container String destPath = System.getenv("IT_PROJECT_DIR"); - LOG.info("Copy doris log file to {}", destPath); - - String feTarPath = "/doris-be.tar"; - String beTarPath = "/doris-fe.tar"; - - // Pack the jar files - container.execInContainer("tar", "cf", feTarPath, DORIS_BE_PATH); - container.execInContainer("tar", "cf", beTarPath, DORIS_FE_PATH); - - container.copyFileFromContainer(feTarPath, destPath + File.separator + "doris-be.tar"); - container.copyFileFromContainer(beTarPath, destPath + File.separator + "doris-fe.tar"); + if (destPath == null || destPath.isEmpty()) { + LOG.warn("IT_PROJECT_DIR is not set, skipping Doris log copy"); + return; + } + LOG.info("Copy doris log files to {}", destPath); + copyContainerLog(FE_SERVICE, DORIS_FE_PATH, destPath, "doris-fe"); + for (int i = 1; i <= BE_SCALE; i++) { + copyContainerLog(BE_SERVICE + "-" + i, DORIS_BE_PATH, destPath, "doris-be-" + i); + } } catch (Exception e) { - LOG.error("Failed to copy container log to local", e); + LOG.error("Failed to copy Doris container logs to local", e); + } + } + + private void copyContainerLog(String serviceName, String logPath, String destPath, String tarName) + throws Exception { + Optional<ContainerState> container = composeContainer.getContainerByServiceName(serviceName); + if (container.isPresent()) { + String tarPath = "/" + tarName + ".tar"; + container.get().execInContainer("tar", "cf", tarPath, logPath); + container.get().copyFileFromContainer(tarPath, destPath + File.separator + tarName + ".tar"); + } else { + LOG.warn("Doris {} container not found, skipping log copy", serviceName); } } + @Override + public String getContainerIpAddress() { + return feIpAddress; + } + @Override protected boolean checkContainerStatus(int retryLimit) { - String dorisJdbcUrl = format("jdbc:mysql://%s:%d/", getContainerIpAddress(), FE_MYSQL_PORT); - LOG.info("Doris url is " + dorisJdbcUrl); + String dorisJdbcUrl = format("jdbc:mysql://%s:%d/", feIpAddress, FE_MYSQL_PORT); + LOG.info("Doris JDBC url is {}", dorisJdbcUrl); await() - .atMost(30, TimeUnit.SECONDS) - .pollInterval(30 / retryLimit, TimeUnit.SECONDS) + .atMost(120, TimeUnit.SECONDS) + .pollInterval(120 / retryLimit, TimeUnit.SECONDS) .until( () -> { try (Connection connection = DriverManager.getConnection(dorisJdbcUrl, USER_NAME, ""); Statement statement = connection.createStatement()) { - // execute `SHOW PROC '/backends';` to check if backends is ready + // execute `SHOW PROC '/backends';` to check if all backends are ready String query = "SHOW PROC '/backends';"; + int aliveCount = 0; try (ResultSet resultSet = statement.executeQuery(query)) { while (resultSet.next()) { + LOG.info("Test " + resultSet.toString()); Review Comment: `LOG.info("Test " + resultSet.toString())` inside the backend polling loop will spam logs and `ResultSet#toString()` is not meaningful for diagnosing readiness. Please remove this debug log or replace it with specific columns (and use parameterized logging) if detailed readiness diagnostics are needed. ```suggestion ``` ########## integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/DorisContainer.java: ########## @@ -63,78 +84,130 @@ protected DorisContainer( Map<String, String> envVars, Optional<Network> network) { super(image, hostName, ports, extraHosts, filesToMount, envVars, network); - } - @Override - protected void setupContainer() { - super.setupContainer(); - withLogConsumer(new PrintingContainerLog(format("%-14s| ", "DorisContainer"))); - withStartupTimeout(Duration.ofMinutes(5)); + String dir = System.getenv("GRAVITINO_ROOT_DIR"); + if (dir == null || dir.isEmpty()) { + throw new RuntimeException("GRAVITINO_ROOT_DIR is not set"); + } + String composeFile = + ITUtils.joinPath( + dir, "integration-test-common", "doris-docker-script", "docker-compose.yaml"); + + composeContainer = + new ComposeContainer(new File(composeFile)) + .withEnv("GRAVITINO_CI_DORIS_DOCKER_IMAGE", DEFAULT_IMAGE) Review Comment: `ComposeContainer.withEnv("GRAVITINO_CI_DORIS_DOCKER_IMAGE", DEFAULT_IMAGE)` will receive `null` if `GRAVITINO_CI_DORIS_DOCKER_IMAGE` is unset, which can lead to an NPE or to docker-compose using an empty image value. Please fail fast with a clear validation message or provide a default image (e.g., the Doris version referenced in the PR description). ```suggestion String effectiveImage = (DEFAULT_IMAGE != null && !DEFAULT_IMAGE.isEmpty()) ? DEFAULT_IMAGE : image; Preconditions.checkArgument( effectiveImage != null && !effectiveImage.isEmpty(), "Doris Docker image must be configured via GRAVITINO_CI_DORIS_DOCKER_IMAGE or " + "DorisContainer builder image"); composeContainer = new ComposeContainer(new File(composeFile)) .withEnv("GRAVITINO_CI_DORIS_DOCKER_IMAGE", effectiveImage) ``` ########## integration-test-common/src/test/java/org/apache/gravitino/integration/test/container/DorisContainer.java: ########## @@ -63,78 +84,130 @@ protected DorisContainer( Map<String, String> envVars, Optional<Network> network) { super(image, hostName, ports, extraHosts, filesToMount, envVars, network); - } Review Comment: Even though this class delegates to `ComposeContainer`, it still calls `super(...)`, which eagerly constructs and configures a `GenericContainer` (and requires a non-null `image`) that is never started or used. This adds unnecessary overhead and makes the compose-based container indirectly depend on BaseContainer’s image semantics; consider refactoring to avoid creating the unused `GenericContainer` (e.g., not extending `BaseContainer`, or adding a compose-specific base/contract). ########## integration-test-common/doris-docker-script/start-be.sh: ########## @@ -0,0 +1,87 @@ +#!/bin/bash +# +# 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. +# + +# BE-only startup script for docker-compose multi-container deployment. +# This overrides the image's default start.sh which starts both FE and BE. + +DORIS_HOME="$(dirname "${BASH_SOURCE-$0}")" +DORIS_HOME="$(cd "${DORIS_HOME}" >/dev/null; pwd)" +DORIS_BE_HOME="${DORIS_HOME}/be/" + +# Patch BE startup script: skip vm.max_map_count and ulimit checks, remove slow chmod +DORIS_BE_SCRIPT="${DORIS_BE_HOME}/bin/start_be.sh" +sed -i '/Please set vm.max_map_count/,/exit 1/{s/exit 1/#exit 1\n echo "skip this"/}' ${DORIS_BE_SCRIPT} +sed -i '/Please set the maximum number of open file descriptors/,/exit 1/{s/exit 1/#exit 1\n echo "skip this"/}' ${DORIS_BE_SCRIPT} +sed -i 's/chmod 755 "${DORIS_HOME}\/lib\/doris_be"/#&/' ${DORIS_BE_SCRIPT} + +# Configure priority_networks and report interval in be.conf +CONTAINER_IP=$(hostname -i) +PRIORITY_NETWORKS=$(echo "${CONTAINER_IP}" | awk -F '.' '{print$1"."$2"."$3".0/24"}') +echo "add priority_networks = ${PRIORITY_NETWORKS} to be.conf" +echo "priority_networks = ${PRIORITY_NETWORKS}" >> ${DORIS_BE_HOME}/conf/be.conf +echo "report_disk_state_interval_seconds = 10" >> ${DORIS_BE_HOME}/conf/be.conf + +# Start only BE in daemon mode +${DORIS_BE_HOME}/bin/start_be.sh --daemon + +# Wait for BE to be ready +be_started=false +for i in {1..20}; do + sleep 5 + url="localhost:8040/api/health" + echo "check be for the $i times" + response=$(curl --silent --request GET $url) + + if echo "$response" | grep -q '"status": "OK"'; then + be_started=true + echo "Doris BE started successfully, response: $response" + break + else + echo "check failed, response: $response" + fi +done + +if [ "$be_started" = false ]; then + echo "ERROR: Doris BE failed to start" +fi + +# Register this BE to the central FE +be_added=false +for i in {1..10}; do + echo "add Doris BE to FE for the $i times" + + mysql -h doris-fe -P9030 -uroot -e "ALTER SYSTEM ADD BACKEND '${CONTAINER_IP}:9050'" + if [ $? -ne 0 ]; then + echo "Failed to add the BE to the FE" + else + be_added=true + echo "Doris BE added to FE successfully" + break + fi + + sleep 1 +done + +if [ "$be_added" = false ]; then + echo "ERROR: Doris BE failed to add to FE" +fi + Review Comment: If registering the BE with the FE fails (`be_added` is false), the script only prints an error and then continues to keep the container alive. Please `exit 1` here as well; otherwise the compose environment may look "up" while the cluster can never become ready. ########## integration-test-common/doris-docker-script/docker-compose.yaml: ########## @@ -0,0 +1,62 @@ +# +# 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. + +services: + + doris-fe: + image: ${GRAVITINO_CI_DORIS_DOCKER_IMAGE} + hostname: doris-fe Review Comment: The compose file uses `image: ${GRAVITINO_CI_DORIS_DOCKER_IMAGE}` without a default. If the env var is missing/empty, `docker-compose` will fail with an invalid image reference. Consider adding a default value in the YAML (e.g., `${GRAVITINO_CI_DORIS_DOCKER_IMAGE:-apache/doris:2.1.7}`) to match the PR description and make local runs more reliable. ########## integration-test-common/doris-docker-script/docker-compose.yaml: ########## @@ -0,0 +1,62 @@ +# +# 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. + +services: + + doris-fe: + image: ${GRAVITINO_CI_DORIS_DOCKER_IMAGE} + hostname: doris-fe + networks: + - doris-net + volumes: + - ./start-fe.sh:/opt/apache-doris/start-fe.sh + command: ["bash", "start-fe.sh"] + ports: + - "8030:8030" + - "9030:9030" Review Comment: Binding FE ports to fixed host ports (`8030:8030`, `9030:9030`) makes the IT environment fragile (fails if those ports are already in use, and prevents parallel test execution on the same host). Consider removing the `ports:` section and relying on Testcontainers' `withExposedService(...)` port mapping, then use `composeContainer.getServicePort(...)` when constructing JDBC/HTTP URLs. ```suggestion ``` ########## integration-test-common/doris-docker-script/start-fe.sh: ########## @@ -0,0 +1,59 @@ +#!/bin/bash +# +# 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. +# + +# FE-only startup script for docker-compose multi-container deployment. +# This overrides the image's default start.sh which starts both FE and BE. + +DORIS_HOME="$(dirname "${BASH_SOURCE-$0}")" +DORIS_HOME="$(cd "${DORIS_HOME}" >/dev/null; pwd)" +DORIS_FE_HOME="${DORIS_HOME}/fe/" + +# Configure priority_networks based on container IP +CONTAINER_IP=$(hostname -i) +PRIORITY_NETWORKS=$(echo "${CONTAINER_IP}" | awk -F '.' '{print$1"."$2"."$3".0/24"}') +echo "add priority_networks = ${PRIORITY_NETWORKS} to fe.conf" +echo "priority_networks = ${PRIORITY_NETWORKS}" >> ${DORIS_FE_HOME}/conf/fe.conf + +# Start only FE in daemon mode +${DORIS_FE_HOME}/bin/start_fe.sh --daemon + +# Wait for FE to be ready +fe_started=false +for i in {1..20}; do + sleep 5 + url="http://localhost:8030/api/bootstrap"; + echo "check fe for the $i times" + response=$(curl --silent --request GET $url) + + if echo "$response" | grep -q '"msg":"success"'; then + fe_started=true + echo "Doris FE started successfully, response: $response" + break + else + echo "check failed, response: $response" + fi +done + +if [ "$fe_started" = false ]; then + echo "ERROR: Doris FE failed to start" Review Comment: If FE fails to start, the script only prints an error and then continues to keep the container alive, which can hide failures and slow down debugging. Please `exit 1` (non-zero) when `fe_started` is false so docker-compose/Testcontainers fail fast. ```suggestion echo "ERROR: Doris FE failed to start" exit 1 ``` -- 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]
