zabetak commented on code in PR #6953: URL: https://github.com/apache/hive/pull/6953#discussion_r4539210707
########## dev-support/precommit-init-metastore.sh: ########## @@ -0,0 +1,120 @@ +#!/usr/bin/env 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. +# + +# Initializes an empty metastore, and the Hive SYS/INFORMATION_SCHEMA databases on top of it, with +# bin/schematool of the binary distribution built by "mvn install -Pdist". +# Used by the init@<db> stages of the Jenkinsfile, in the precommit image (dev-support/docker/precommit). +# +# Usage: dev-support/precommit-init-metastore.sh <derby|postgres> +# +# Environment: +# HADOOP_HOME Hadoop installation used by bin/schematool (default: /opt/hadoop) +# MAVEN_ARGS extra Maven arguments (settings, local repository) used to fetch the JDBC driver +# DOCKER_NETWORK network of the database container (default: host); its port must be +# reachable on localhost +# POSTGRES_IMAGE image of the postgres container (default: postgres:17-alpine) + +set -euo pipefail + +dbType=${1:?usage: $0 <derby|postgres>} + +cd "$(dirname "$0")/.." + +hiveHomes=(packaging/target/apache-hive-*-bin/apache-hive-*-bin) +if [[ ! -d ${hiveHomes[0]} ]]; then + echo "No Hive binary distribution under packaging/target; build it with -Pdist first." >&2 + exit 1 +fi +export HIVE_HOME=$PWD/${hiveHomes[0]} +export HADOOP_HOME=${HADOOP_HOME:-/opt/hadoop} +export HADOOP_CLIENT_OPTS="${HADOOP_CLIENT_OPTS:-} --add-opens java.base/java.net=ALL-UNNAMED" + +work=$PWD/target/init-metastore/$dbType +rm -rf "$work" +mkdir -p "$work/conf" "$work/lib" +export HIVE_CONF_DIR=$work/conf +export HADOOP_CLIENT_OPTS+=" -Dderby.stream.error.file=$work/derby.log" +export HADOOP_CLASSPATH="$work/lib/*" + +dbContainer= +cleanup() { + if [[ -n $dbContainer ]]; then + docker logs "$dbContainer" > "$work/$dbContainer.log" 2>&1 || true + docker rm -f -v "$dbContainer" > /dev/null 2>&1 || true + fi +} +trap cleanup EXIT + +pomProperty() { + xmlstarlet sel -t -v "/_:project/_:properties/_:$1" pom.xml +} + +case $dbType in + derby) + driver=org.apache.derby.jdbc.EmbeddedDriver + url="jdbc:derby:;databaseName=$work/metastore_db;create=true" + user=APP + password=mine + ;; + postgres) + mvn -B -q dependency:copy -Dartifact="org.postgresql:postgresql:$(pomProperty postgres.version)" \ + -DoutputDirectory="$work/lib" + dbContainer=hive-precommit-postgres + docker rm -f -v "$dbContainer" > /dev/null 2>&1 || true + docker run -d --name "$dbContainer" --network "${DOCKER_NETWORK:-host}" \ + -e POSTGRES_USER=hive -e POSTGRES_PASSWORD=hive -e POSTGRES_DB=metastore \ + "${POSTGRES_IMAGE:-postgres:17-alpine}" > /dev/null + # Probe over TCP: during its first start the entrypoint runs a temporary server on the unix socket only. + for ((i = 0; i < 120; i++)); do + docker exec "$dbContainer" pg_isready -q -h 127.0.0.1 -U hive -d metastore && break + sleep 1 + done + docker exec "$dbContainer" pg_isready -h 127.0.0.1 -U hive -d metastore + driver=org.postgresql.Driver + url=jdbc:postgresql://localhost:5432/metastore + user=hive + password=hive + ;; + *) + echo "Unsupported database type: $dbType" >&2 + exit 1 + ;; +esac + +property() { + printf ' <property><name>%s</name><value>%s</value></property>\n' "$1" "$2" +} +{ + echo '<configuration>' + property javax.jdo.option.ConnectionDriverName "$driver" + property javax.jdo.option.ConnectionURL "$url" + property javax.jdo.option.ConnectionUserName "$user" + property javax.jdo.option.ConnectionPassword "$password" + property hive.metastore.warehouse.dir "file://$work/warehouse" + property hive.metastore.warehouse.external.dir "file://$work/warehouse-external" + property hive.exec.scratchdir "$work/scratch" + property hive.exec.local.scratchdir "$work/local-scratch" + property hive.querylog.location "$work/querylog" + property hive.server2.logging.operation.log.location "$work/operation-logs" + property hive.user.install.directory "file://$work/user" + echo '</configuration>' +} > "$HIVE_CONF_DIR/hive-site.xml" + +"$HIVE_HOME/bin/schematool" -verbose -dbType "$dbType" -initSchema Review Comment: This part is already covered by `org.apache.hadoop.hive.metastore.dbinstall.DbInstallBase` and covers more dbtypes than just Postgres and Derby. ########## dev-support/precommit-init-metastore.sh: ########## @@ -0,0 +1,120 @@ +#!/usr/bin/env 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. +# + +# Initializes an empty metastore, and the Hive SYS/INFORMATION_SCHEMA databases on top of it, with +# bin/schematool of the binary distribution built by "mvn install -Pdist". Review Comment: I don't think we gain much by this test of the schematool on the binary distribution. The main difference with `DbInstallBase` is that it runs with a different classpath but there is no guarantee that users are actually setting up their classpath in the same way. If we want to have smoketests on the metastore it would make much more sense to base them on the existing HMS/HS2 images that we already build and publish elsewhere where there is an actual Hive deployment. I guess by dropping these tests we can also get rid of the Hadoop installation simplifying even more the runner image. From my perspective the tiny bit of extra coverage that we have here is not worth the extra complexity and maintenance overhead and I would be very much in favor of dropping those. ########## dev-support/docker/precommit/README.md: ########## @@ -0,0 +1,199 @@ +<!-- +{% comment %} +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. +{% endcomment %} +--> +# Hive precommit image + +The [Jenkinsfile](../../../Jenkinsfile) runs every precommit stage in this image, with a +`docker:dind` sidecar for the tests that start databases in containers. + +The image is kept small on purpose. It holds: + +* Debian (slim), with the GNU shell tools that scripts and tests expect. It is glibc based on + purpose: tests run prebuilt native code that is linked against glibc, such as the Kudu + mini cluster. +* a JDK (Eclipse Temurin), without jmods and man pages +* Maven +* the client part of Hadoop, used only by `bin/schematool` in the `init@<db>` stages + (see [precommit-init-metastore.sh](../../precommit-init-metastore.sh)) +* the docker CLI, git, openssl, python3, rsync, tini and xmlstarlet + +Everything else (Tez, the JDBC drivers, the Hive build dependencies) is fetched by Maven +during the build. Before adding a package, check whether Maven can provide it instead. + +## How the image is published + +The [precommit-image](../../../.github/workflows/precommit-image.yml) workflow builds the +image and smoke tests it on every pull request that changes this directory. When such a +change lands on master, the workflow publishes the image for `linux/amd64` and `linux/arm64` +as `apache/hive:precommit`. That tag is the `precommitImage` of the Jenkinsfile, and +the precommit job pulls it again on every run. The tag is overwritten on every publish: +once the change is merged, every precommit run, on every branch, uses the new image. + +## Changing the image, step by step Review Comment: I am not sure if anyone will bother going through steps 3 to 7. It is probably faster to build and publish the image on a personal account and then launch a precommit run to see if it works. ########## Jenkinsfile: ########## @@ -31,9 +31,13 @@ properties([ parameters([ string(name: 'SPLIT', defaultValue: '22', description: 'Number of buckets to split tests into.'), string(name: 'OPTS', defaultValue: '', description: 'additional maven opts'), + string(name: 'IMAGE', defaultValue: '', description: 'precommit image to use instead of the default one (e.g. a candidate built from dev-support/docker/precommit)'), ]) ]) +// Built from dev-support/docker/precommit/Dockerfile and pushed by .github/workflows/precommit-image.yml. +this.precommitImage = 'apache/hive-ci-runner' Review Comment: Do we want to pin down the image to a specific version/hash/tag to control when changes are made and also protect against accidental pushes to registry? ########## .github/workflows/precommit-image.yml: ########## @@ -0,0 +1,85 @@ +# 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. + +# Builds the precommit image (dev-support/docker/precommit) and smoke tests it. On master it +# publishes the image as apache/hive-ci-runner, the image the Jenkinsfile runs in. +name: Precommit image + +on: + pull_request: + paths: + - 'dev-support/docker/precommit/**' + - '.github/workflows/precommit-image.yml' + push: + branches: + - master + paths: + - 'dev-support/docker/precommit/**' + - '.github/workflows/precommit-image.yml' + workflow_dispatch: + inputs: + tag: + description: 'Tag to publish; use a custom one (e.g. HIVE-12345) to try an image with the IMAGE parameter of the precommit job' + required: true + default: 'latest' + +permissions: + contents: read + +jobs: + build: + runs-on: ubuntu-latest + env: + IMAGE: ${{ github.repository_owner }}/hive-ci-runner:${{ github.event.inputs.tag || 'latest' }} + steps: + - name: Checkout + uses: actions/checkout@v6 + with: + persist-credentials: false + + - name: Set up QEMU + uses: docker/setup-qemu-action@99012661954931238ded8c8b007157a8430204e1 # v4.4.0 + + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@f87e5991a6d7451dcb8d9637bfbc97413f497069 # v4.4.1 + + - name: Build the image + uses: docker/build-push-action@c3c9e263c25d99ce0380d002d59b67737d91b0dc # v7.4.0 + with: + context: ./dev-support/docker/precommit/ + load: true + tags: hive-precommit:test + + - name: Smoke test the image + run: | + docker images hive-precommit:test + dev-support/docker/precommit/smoke-test.sh hive-precommit:test + + - name: Login to Docker Hub + if: github.event_name != 'pull_request' && github.repository == 'apache/hive' Review Comment: Do we want to restrict login/publish to apache/hive repository? Maybe a contributor wants to test the action on their fork would that be a problem? Were you able to test these parts of the action somehow? ########## dev-support/precommit-init-metastore.sh: ########## @@ -0,0 +1,120 @@ +#!/usr/bin/env 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. +# + +# Initializes an empty metastore, and the Hive SYS/INFORMATION_SCHEMA databases on top of it, with +# bin/schematool of the binary distribution built by "mvn install -Pdist". +# Used by the init@<db> stages of the Jenkinsfile, in the precommit image (dev-support/docker/precommit). +# +# Usage: dev-support/precommit-init-metastore.sh <derby|postgres> +# +# Environment: +# HADOOP_HOME Hadoop installation used by bin/schematool (default: /opt/hadoop) +# MAVEN_ARGS extra Maven arguments (settings, local repository) used to fetch the JDBC driver +# DOCKER_NETWORK network of the database container (default: host); its port must be +# reachable on localhost +# POSTGRES_IMAGE image of the postgres container (default: postgres:17-alpine) + +set -euo pipefail + +dbType=${1:?usage: $0 <derby|postgres>} + +cd "$(dirname "$0")/.." + +hiveHomes=(packaging/target/apache-hive-*-bin/apache-hive-*-bin) +if [[ ! -d ${hiveHomes[0]} ]]; then + echo "No Hive binary distribution under packaging/target; build it with -Pdist first." >&2 + exit 1 +fi +export HIVE_HOME=$PWD/${hiveHomes[0]} +export HADOOP_HOME=${HADOOP_HOME:-/opt/hadoop} +export HADOOP_CLIENT_OPTS="${HADOOP_CLIENT_OPTS:-} --add-opens java.base/java.net=ALL-UNNAMED" + +work=$PWD/target/init-metastore/$dbType +rm -rf "$work" +mkdir -p "$work/conf" "$work/lib" +export HIVE_CONF_DIR=$work/conf +export HADOOP_CLIENT_OPTS+=" -Dderby.stream.error.file=$work/derby.log" +export HADOOP_CLASSPATH="$work/lib/*" + +dbContainer= +cleanup() { + if [[ -n $dbContainer ]]; then + docker logs "$dbContainer" > "$work/$dbContainer.log" 2>&1 || true + docker rm -f -v "$dbContainer" > /dev/null 2>&1 || true + fi +} +trap cleanup EXIT + +pomProperty() { + xmlstarlet sel -t -v "/_:project/_:properties/_:$1" pom.xml +} + +case $dbType in + derby) + driver=org.apache.derby.jdbc.EmbeddedDriver + url="jdbc:derby:;databaseName=$work/metastore_db;create=true" + user=APP + password=mine + ;; + postgres) + mvn -B -q dependency:copy -Dartifact="org.postgresql:postgresql:$(pomProperty postgres.version)" \ + -DoutputDirectory="$work/lib" + dbContainer=hive-precommit-postgres + docker rm -f -v "$dbContainer" > /dev/null 2>&1 || true + docker run -d --name "$dbContainer" --network "${DOCKER_NETWORK:-host}" \ + -e POSTGRES_USER=hive -e POSTGRES_PASSWORD=hive -e POSTGRES_DB=metastore \ + "${POSTGRES_IMAGE:-postgres:17-alpine}" > /dev/null + # Probe over TCP: during its first start the entrypoint runs a temporary server on the unix socket only. + for ((i = 0; i < 120; i++)); do + docker exec "$dbContainer" pg_isready -q -h 127.0.0.1 -U hive -d metastore && break + sleep 1 + done + docker exec "$dbContainer" pg_isready -h 127.0.0.1 -U hive -d metastore + driver=org.postgresql.Driver + url=jdbc:postgresql://localhost:5432/metastore + user=hive + password=hive + ;; + *) + echo "Unsupported database type: $dbType" >&2 + exit 1 + ;; +esac + +property() { + printf ' <property><name>%s</name><value>%s</value></property>\n' "$1" "$2" +} +{ + echo '<configuration>' + property javax.jdo.option.ConnectionDriverName "$driver" + property javax.jdo.option.ConnectionURL "$url" + property javax.jdo.option.ConnectionUserName "$user" + property javax.jdo.option.ConnectionPassword "$password" + property hive.metastore.warehouse.dir "file://$work/warehouse" + property hive.metastore.warehouse.external.dir "file://$work/warehouse-external" + property hive.exec.scratchdir "$work/scratch" + property hive.exec.local.scratchdir "$work/local-scratch" + property hive.querylog.location "$work/querylog" + property hive.server2.logging.operation.log.location "$work/operation-logs" + property hive.user.install.directory "file://$work/user" + echo '</configuration>' +} > "$HIVE_CONF_DIR/hive-site.xml" + +"$HIVE_HOME/bin/schematool" -verbose -dbType "$dbType" -initSchema +"$HIVE_HOME/bin/schematool" -verbose -dbType hive -initSchema -metaDbType "$dbType" Review Comment: I have the impression that with a small addition somewhere in `org.apache.hadoop.hive.metastore.dbinstall.DbInstallBase` we could also add coverage for `SYS/INFORMATION_SCHEMA` initialization. ########## dev-support/docker/precommit/smoke-test.sh: ########## @@ -0,0 +1,75 @@ +#!/usr/bin/env 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. +# + +# Checks that a precommit image provides what the Jenkinsfile and the tests rely on. Review Comment: Requirements are very likely to change as the project involves and nobody will ever bother to update and remove entries from this file. The opposite is more likely to happen meaning that we end up keeping useless stuff just because this test fails. In most cases, people who want to update the image will just raise a PR and run it multiple times till everything passes. I don't feel strongly about removing it but I could certainly leave without :) ########## Jenkinsfile: ########## @@ -31,9 +31,13 @@ properties([ parameters([ string(name: 'SPLIT', defaultValue: '22', description: 'Number of buckets to split tests into.'), string(name: 'OPTS', defaultValue: '', description: 'additional maven opts'), + string(name: 'IMAGE', defaultValue: '', description: 'precommit image to use instead of the default one (e.g. a candidate built from dev-support/docker/precommit)'), ]) ]) +// Built from dev-support/docker/precommit/Dockerfile and pushed by .github/workflows/precommit-image.yml. +this.precommitImage = 'apache/hive:precommit' Review Comment: If we settle on `hive-ci-runner` then maybe we should change the directory structure to `dev-support/docker/ci/runner` and then potentially bring `hive-ci-jenkins` image under `dev-support/docker/ci/jenkins` in a subsequent PR. Another idea would be another top-level dir for `ci` and then have `ci/docker/runner`. Anyways these are kinda of details so pick whatever makes more sense (even as it is is fine). -- 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]
