Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/24372 )
Change subject: IMPALA-15043: Add Kubernetes-in-Docker E2E scripts ...................................................................... Patch Set 47: (10 comments) http://gerrit.cloudera.org:8080/#/c/24372/47/bin/jenkins/all-tests.sh File bin/jenkins/all-tests.sh: http://gerrit.cloudera.org:8080/#/c/24372/47/bin/jenkins/all-tests.sh@110 PS47, Line 110: if [[ "$K8S_E2E_TEST" == true ]]; then : if ! bin/jenkins/run-k8s-e2e-tests.sh; then : RET_CODE=1 : fi : fi This code would make more sense in bin/run-all-tests.sh unless there is a reason to keep it here? http://gerrit.cloudera.org:8080/#/c/24372/47/bin/jenkins/run-k8s-e2e-tests.sh File bin/jenkins/run-k8s-e2e-tests.sh: http://gerrit.cloudera.org:8080/#/c/24372/47/bin/jenkins/run-k8s-e2e-tests.sh@1 PS47, Line 1: #!/bin/bash Use this instead: #!/usr/bin/env bash http://gerrit.cloudera.org:8080/#/c/24372/47/bin/run-k8s-e2e-tests.sh File bin/run-k8s-e2e-tests.sh: http://gerrit.cloudera.org:8080/#/c/24372/47/bin/run-k8s-e2e-tests.sh@23 PS47, Line 23: IMPALA_HOME="$(cd "${SCRIPT_DIR}/.." && pwd)" Don't override IMPALA_HOME if it is already set. http://gerrit.cloudera.org:8080/#/c/24372/47/bin/run-k8s-e2e-tests.sh@24 PS47, Line 24: export IMPALA_HOME Should this script also run cd "${IMPALA_HOME}" http://gerrit.cloudera.org:8080/#/c/24372/47/bin/run-k8s-e2e-tests.sh@67 PS47, Line 67: python3 Use "${IMPALA_HOME}/bin/impala-python3" command instead. http://gerrit.cloudera.org:8080/#/c/24372/47/bin/run-k8s-e2e-tests.sh@131 PS47, Line 131: if [[ -n "${PORT_FORWARD_PID}" ]] && kill -0 "${PORT_FORWARD_PID}" 2>/dev/null; then : kill "${PORT_FORWARD_PID}" >/dev/null 2>&1 || true : fi Nit: instead of duplicating this code, can the cleanup() function be called instead? http://gerrit.cloudera.org:8080/#/c/24372/47/bin/run-k8s-e2e-tests.sh@187 PS47, Line 187: "${K8S_TEST_TARGET}" It's also frequently useful to be able to provide the '-k', '-x', and '--pdb' (plus other) flags to the 'run-tests.py' script. Can this script support passing additional arguments to 'run-tests.py'? http://gerrit.cloudera.org:8080/#/c/24372/47/docs/kubernetes-in-docker-e2e.md File docs/kubernetes-in-docker-e2e.md: http://gerrit.cloudera.org:8080/#/c/24372/47/docs/kubernetes-in-docker-e2e.md@1 PS47, Line 1: <!-- The 'docs' folder is for templates that are parsed into the html files for impala.apache.org/docs. This README file does not belong in the `docs' folder. http://gerrit.cloudera.org:8080/#/c/24372/47/helm/impala/README.md File helm/impala/README.md: http://gerrit.cloudera.org:8080/#/c/24372/47/helm/impala/README.md@407 PS47, Line 407: impala-impala-impalad Is this a typo? http://gerrit.cloudera.org:8080/#/c/24372/47/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/24372/47/tests/common/impala_test_suite.py@493 PS47, Line 493: try: Rather than modifying this file, set ENABLE_BEESWAX=false in the test execution script. -- To view, visit http://gerrit.cloudera.org:8080/24372 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I122b99c38c8c70ab535349f92212621330f2aa55 Gerrit-Change-Number: 24372 Gerrit-PatchSet: 47 Gerrit-Owner: Anubhav Jindal <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Anubhav Jindal <[email protected]> Gerrit-Reviewer: Gokul Kolady <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Tue, 04 Aug 2026 19:13:20 +0000 Gerrit-HasComments: Yes
