paul-rogers commented on code in PR #13681:
URL: https://github.com/apache/druid/pull/13681#discussion_r1073851602


##########
.github/workflows/reusable-revised-its.yml:
##########
@@ -0,0 +1,107 @@
+# 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.
+
+name: Revised Integration Tests shared workflow
+on:
+  workflow_call:
+    inputs:
+      script:
+        description: 'Which IT script to run'
+        required: true
+        type: string
+      build_jdk:
+        description: 'Which jdk version was used to build'
+        required: true
+        type: string
+      runtime_jdk:
+        description: 'Which JDK version to use at runtime'
+        required: true
+        type: string
+      use_indexer:
+        description: 'Which indexer to use'
+        required: true
+        type: string
+      druid_it_image_name:
+        required: true
+        type: string
+
+env:
+  MVN: mvn -B
+  MAVEN_SKIP: -P skip-static-checks -Dweb.console.skip=true 
-Dmaven.javadoc.skip=true
+  MAVEN_SKIP_TESTS: -P skip-tests
+  MAVEN_OPTS: -Xmx3000m

Review Comment:
   The `MAVEN_*` variables may not actually be needed here. Assuming we're 
using the `it.sh` script, we don't actually invoke Maven directly for the new 
ITs. Instead, `it.sh` does it for us and these various options are coded into 
`it.sh`, but only if we do the `it.sh build` command, which we probably should 
_not_ do since Druid (and the image) are already built.
   
   No harm in setting these: it's just that nothing will (probably) use them.



##########
.github/workflows/it.yml:
##########
@@ -0,0 +1,145 @@
+# 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.
+
+on:
+  workflow_call
+
+jobs:
+  integration-index-tests-middleManager:
+    strategy:
+      fail-fast: false
+      max-parallel: 3
+      matrix:
+        jdk: [8, 11]
+        testing_group: [batch-index, input-format, input-source, 
perfect-rollup-parallel-batch-index, kafka-index, kafka-index-slow, 
kafka-transactional-index, kafka-transactional-index-slow, kafka-data-format, 
ldap-security, realtime-index, append-ingestion, compaction]
+    name:  (Compile=jdk${{ matrix.jdk }}, Run=jdk${{ matrix.jdk }}, 
Indexer=middleManager) ${{ matrix.testing_group }} integration test
+    uses: 
tejaswini-imply/druid/.github/workflows/reusable-standard-its.yml@update_uts_gha
+    with:
+      build_jdk: 8
+      runtime_jdk: ${{ matrix.jdk }}
+      testing_groups: -Dgroups=${{ matrix.testing_group }}
+      use_indexer: middleManager
+
+  integration-index-tests-indexer:
+    strategy:
+      fail-fast: false
+      max-parallel: 3
+      matrix:
+        jdk: [8, 11]
+        testing_group: [input-source, perfect-rollup-parallel-batch-index, 
kafka-index, kafka-transactional-index, kafka-index-slow, 
kafka-transactional-index-slow, kafka-data-format, append-ingestion, compaction]
+    name:  (Compile=jdk${{ matrix.jdk }}, Run=jdk${{ matrix.jdk }}, 
Indexer=indexer) ${{ matrix.testing_group }} integration test
+    uses: 
tejaswini-imply/druid/.github/workflows/reusable-standard-its.yml@update_uts_gha
+    with:
+      build_jdk: 8
+      runtime_jdk: ${{ matrix.jdk }}
+      testing_groups: -Dgroups=${{ matrix.testing_group }}
+      use_indexer: indexer
+
+  integration-query-tests-middleManager:
+    strategy:
+      fail-fast: false
+      max-parallel: 3
+      matrix:
+        jdk: [8, 11]
+        testing_group: [query, query-retry, query-error, security, 
high-availability]
+    name:  (Compile=jdk${{ matrix.jdk }}, Run=jdk${{ matrix.jdk }}, 
Indexer=middleManager) ${{ matrix.testing_group }} integration test
+    uses: 
tejaswini-imply/druid/.github/workflows/reusable-standard-its.yml@update_uts_gha
+    with:
+      build_jdk: 8
+      runtime_jdk: ${{ matrix.jdk }}
+      testing_groups: -Dgroups=${{ matrix.testing_group }}
+      use_indexer: middleManager
+      override_config_path: ./environment-configs/test-groups/prepopulated-data
+
+  integration-query-tests-middleManager-mariaDB:
+    strategy:
+      fail-fast: false
+      matrix:
+        jdk: [8, 11]
+    name:  (Compile=jdk8, Run=jdk${{ matrix.jdk }}, Indexer=middleManager) 
query integration test (mariaDB)
+    uses: 
tejaswini-imply/druid/.github/workflows/reusable-standard-its.yml@update_uts_gha
+    with:
+      build_jdk: 8
+      runtime_jdk: ${{ matrix.jdk }}
+      testing_groups: -Dgroups=query
+      use_indexer: middleManager
+      mysql_driver: org.mariadb.jdbc.Driver
+      override_config_path: ./environment-configs/test-groups/prepopulated-data
+
+  integration-shuffle-deep-store-tests:
+    strategy:
+      fail-fast: false
+      matrix:
+        indexer: [indexer, middleManager]
+    name:  (Compile=openjdk8, Run=openjdk8, Indexer=${{ matrix.indexer }}) 
perfect rollup parallel batch index integration test with deep storage as 
intermediate store
+    uses: 
tejaswini-imply/druid/.github/workflows/reusable-standard-its.yml@update_uts_gha
+    with:
+      build_jdk: 8
+      runtime_jdk: 8
+      testing_groups: -Dgroups=shuffle-deep-store
+      use_indexer: ${{ matrix.indexer }}
+      override_config_path: 
./environment-configs/test-groups/shuffle-deep-store
+
+  integration-custom-coordinator-duties-tests:
+    name: (Compile=jdk8, Run=jdk8, Indexer=middleManager) custom coordinator 
duties integration test
+    uses: 
tejaswini-imply/druid/.github/workflows/reusable-standard-its.yml@update_uts_gha
+    with:
+      build_jdk: 8
+      runtime_jdk: 8
+      testing_groups: -Dgroups=custom-coordinator-duties
+      use_indexer: middleManager
+      override_config_path: 
./environment-configs/test-groups/custom-coordinator-duties
+
+  integration-other-tests:
+    strategy:
+      fail-fast: false
+      matrix:
+        jdk: [8, 11]
+        indexer: [middleManager, indexer]
+    name:  (Compile=jdk${{ matrix.jdk }}, Run=jdk${{ matrix.jdk }}, 
Indexer=${{ matrix.indexer }}) other integration tests
+    uses: 
tejaswini-imply/druid/.github/workflows/reusable-standard-its.yml@update_uts_gha
+    with:
+      build_jdk: 8
+      runtime_jdk: ${{ matrix.jdk }}
+      testing_groups: 
-DexcludedGroups=batch-index,input-format,input-source,perfect-rollup-parallel-batch-index,kafka-index,query,query-retry,query-error,realtime-index,security,ldap-security,s3-deep-storage,gcs-deep-storage,azure-deep-storage,hdfs-deep-storage,s3-ingestion,kinesis-index,kinesis-data-format,kafka-transactional-index,kafka-index-slow,kafka-transactional-index-slow,kafka-data-format,hadoop-s3-to-s3-deep-storage,hadoop-s3-to-hdfs-deep-storage,hadoop-azure-to-azure-deep-storage,hadoop-azure-to-hdfs-deep-storage,hadoop-gcs-to-gcs-deep-storage,hadoop-gcs-to-hdfs-deep-storage,aliyun-oss-deep-storage,append-ingestion,compaction,high-availability,upgrade,shuffle-deep-store,custom-coordinator-duties
+      use_indexer: ${{ matrix.indexer }}
+
+  integration-batch_index_k8s:
+    name: (Compile=jdk8, Run=jdk8, Cluster Build On K8s) 
ITNestedQueryPushDownTest integration test
+    runs-on: ubuntu-22.04
+    env:
+      MVN: mvn --no-snapshot-updates
+      MAVEN_SKIP: -P skip-static-checks -Dweb.console.skip=true 
-Dmaven.javadoc.skip=true
+      CONFIG_FILE: k8s_run_config_file.json
+      IT_TEST: -Dit.test=ITNestedQueryPushDownTest
+      POD_NAME: int-test
+      POD_NAMESPACE: default
+      BUILD_DRUID_CLUSTER: true
+    steps:
+      - name: Checkout branch
+        uses: actions/checkout@v3
+      - name: Setup java
+        uses: actions/setup-java@v3
+        with:
+          distribution: 'zulu'
+          java-version: 8
+      - name: Restore Maven repository
+        uses: actions/cache@v3
+        with:
+         path: ~/.m2/repository
+         key: maven-${{ runner.os }}-8-${{ hashFiles('**/pom.xml') }}
+      - name: Run IT
+        run: |
+          ${MVN} verify -pl integration-tests -P int-tests-config-file 
${IT_TEST} ${MAVEN_SKIP} -Dpod.name=${POD_NAME} 
-Dpod.namespace=${POD_NAMESPACE} -Dbuild.druid.cluster=${BUILD_DRUID_CLUSTER}

Review Comment:
   Nit: missing newline.
   
   This is for the "old" ITs, right? Might be good to add a comment to that 
effect somewhere in this script.



##########
.github/workflows/unit-and-integration-tests-unified.yml:
##########
@@ -0,0 +1,160 @@
+# 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.
+
+name: Unit & Integration tests CI
+on:
+  push:
+    branches:
+      - master
+      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
+  pull_request:
+    branches:
+      - master
+      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
+
+concurrency:
+  group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || 
github.head_ref || github.ref }}'
+  cancel-in-progress: true
+
+jobs:
+  build:
+    strategy:
+      fail-fast: false
+      matrix:
+        jdk: [ '8', '11', '17' ]
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout branch
+        uses: actions/checkout@v3
+      - name: Cache Maven m2 repository
+        id: maven
+        uses: actions/cache@v3
+        with:
+          path: ~/.m2/repository
+          key: maven-${{ runner.os }}-${{ matrix.jdk }}-${{ 
hashFiles('**/pom.xml') }}
+          restore-keys: |
+            maven-${{ runner.os }}-${{ matrix.jdk }}
+      - name: Cache targets
+        id: target
+        uses: actions/cache@v3
+        with:
+          path: |
+            ./target
+            ./extendedset/target
+            ./hll/target
+            ./processing/target
+            ./server/target
+            ./indexing-hadoop/target
+            ./web-console/target
+            ./indexing-service/target
+            ./services/target
+            ./benchmarks/target
+            ./distribution/target
+            ./integration-tests/target
+          key: maven-${{ runner.os }}-${{ matrix.jdk }}-targets-${{ 
hashFiles('**/pom.xml') }}
+          restore-keys: |
+            maven-${{ runner.os }}-${{ matrix.jdk }}-targets
+      - name: Cache image
+        id: docker_container
+        uses: actions/cache@v3
+        with:
+          key: druid-container-jdk${{ matrix.jdk }}.tar.gz-${{ 
hashFiles('**/pom.xml') }}
+          path: |

Review Comment:
   Would be great to add comments here for the non-intuitive stuff. For 
example, we do need to cache that `env.sh` file: it contains info from the 
image build task passed to the test run tasks. Should it be in the cache list 
above? It would be if its target directory were included, but it isn't.



##########
.github/workflows/unit-and-integration-tests-unified.yml:
##########
@@ -0,0 +1,160 @@
+# 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.
+
+name: Unit & Integration tests CI
+on:
+  push:
+    branches:
+      - master
+      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
+  pull_request:
+    branches:
+      - master
+      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
+
+concurrency:
+  group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || 
github.head_ref || github.ref }}'
+  cancel-in-progress: true
+
+jobs:
+  build:
+    strategy:
+      fail-fast: false
+      matrix:
+        jdk: [ '8', '11', '17' ]
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout branch
+        uses: actions/checkout@v3
+      - name: Cache Maven m2 repository
+        id: maven
+        uses: actions/cache@v3
+        with:
+          path: ~/.m2/repository
+          key: maven-${{ runner.os }}-${{ matrix.jdk }}-${{ 
hashFiles('**/pom.xml') }}
+          restore-keys: |
+            maven-${{ runner.os }}-${{ matrix.jdk }}
+      - name: Cache targets
+        id: target
+        uses: actions/cache@v3
+        with:
+          path: |
+            ./target
+            ./extendedset/target
+            ./hll/target
+            ./processing/target
+            ./server/target
+            ./indexing-hadoop/target
+            ./web-console/target
+            ./indexing-service/target
+            ./services/target
+            ./benchmarks/target
+            ./distribution/target
+            ./integration-tests/target
+          key: maven-${{ runner.os }}-${{ matrix.jdk }}-targets-${{ 
hashFiles('**/pom.xml') }}
+          restore-keys: |
+            maven-${{ runner.os }}-${{ matrix.jdk }}-targets
+      - name: Cache image
+        id: docker_container
+        uses: actions/cache@v3
+        with:
+          key: druid-container-jdk${{ matrix.jdk }}.tar.gz-${{ 
hashFiles('**/pom.xml') }}
+          path: |
+            ./druid-container-jdk${{ matrix.jdk }}.tar.gz
+            ./integration-tests-ex/image/target/env.sh
+          restore-keys: |
+            druid-container-jdk${{ matrix.jdk }}.tar.gz-
+      - name: Load docker image
+        if: steps.docker_container.outputs.cache-hit == 'true'
+        run: |
+          docker load --input druid-container-jdk${{ matrix.jdk }}.tar.gz
+          docker images
+      - name: Setup java
+        if: (steps.docker_container.outputs.cache-hit != 'true' || 
steps.target.outputs.cache-hit != 'true')
+        uses: actions/setup-java@v3
+        with:
+          distribution: 'zulu'
+          java-version: ${{ matrix.jdk }}
+      - name: Maven build
+        if: (steps.maven.outputs.cache-hit != 'true' || 
steps.target.outputs.cache-hit != 'true')
+        id: maven_build
+        run: |
+          ./it.sh ci
+      - name: Container build
+        if: (steps.docker_container.outputs.cache-hit != 'true' || 
steps.maven.outputs.cache-hit != 'true' || steps.target.outputs.cache-hit != 
'true')
+        run: |
+          ./it.sh image
+          source ./integration-tests-ex/image/target/env.sh
+          docker tag $DRUID_IT_IMAGE_NAME $DRUID_IT_IMAGE_NAME-jdk${{ 
matrix.jdk }}
+      - name: Set docker image name
+        id: docker_image_name
+        run: |
+          source ./integration-tests-ex/image/target/env.sh
+          echo "DRUID_IT_IMAGE_NAME=$(echo $DRUID_IT_IMAGE_NAME | sed 
's/\./_/g; s/\//_/g; s/\:/-/g')" >> $GITHUB_OUTPUT
+      #- name: Upload image
+      #  if: steps.docker_env.outputs.cache-hit != 'true'
+      #  uses: ishworkh/docker-image-artifact-upload@v1
+      #  with:
+      #    image: "${{ steps.docker_image_name.outputs.DRUID_IT_IMAGE_NAME 
}}-jdk${{ matrix.jdk }}"
+      #    retention_days: "2"
+      - name: Save docker container to archive
+        run: |
+          source ./integration-tests-ex/image/target/env.sh
+          echo $DRUID_IT_IMAGE_NAME
+          docker save "$DRUID_IT_IMAGE_NAME" | gzip > druid-container-jdk${{ 
matrix.jdk }}.tar.gz
+      #- name: Upload image
+      #  if: steps.docker_env.outputs.cache-hit != 'true'
+      #  uses: actions/upload-artifact@v3
+      #  with:
+      #    name: ${{ steps.docker_image_name.outputs.DRUID_IT_IMAGE_NAME 
}}-jdk${{ matrix.jdk }}.tar.gz
+      #    path: ./${{ steps.docker_image_name.outputs.DRUID_IT_IMAGE_NAME 
}}-jdk${{ matrix.jdk }}.tar.gz
+      #    retention-days: "1"
+    outputs:
+      DRUID_IT_IMAGE_NAME: ${{ 
steps.docker_image_name.outputs.DRUID_IT_IMAGE_NAME }}
+
+  unit-tests:
+    strategy:
+      fail-fast: false
+      matrix:
+        sql_compatibility: [ false, true ]
+    name: jdk8, sql-compat=${{ matrix.sql_compatibility }}
+    uses: 
tejaswini-imply/druid/.github/workflows/reusable-unit-tests.yml@update_uts_gha
+    needs: build
+    with:
+      jdk: 8
+      sql_compatibility: ${{ matrix.sql_compatibility }}
+
+  unit-tests-phase2:
+    strategy:
+      fail-fast: false
+      matrix:
+        jdk: [11, 17]
+        sql_compatibility: [ false, true ]
+    name: jdk${{ matrix.jdk }}, sql-compat=${{ matrix.sql_compatibility }}
+    uses: 
tejaswini-imply/druid/.github/workflows/reusable-unit-tests.yml@update_uts_gha
+    needs: unit-tests
+    with:
+      jdk: ${{ matrix.jdk }}
+      sql_compatibility: ${{ matrix.sql_compatibility }}
+
+  its:
+    needs: unit-tests
+    uses: tejaswini-imply/druid/.github/workflows/it.yml@update_uts_gha
+
+  revised-its:
+    needs: unit-tests
+    uses: tejaswini-imply/druid/.github/workflows/it-ex.yml@update_uts_gha
+    with:
+      druid_it_image_name: "${{ needs.build.outputs.DRUID_IT_IMAGE_NAME }}"

Review Comment:
   Nit: missing newline



##########
.github/workflows/unit-and-integration-tests-unified.yml:
##########
@@ -0,0 +1,160 @@
+# 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.
+
+name: Unit & Integration tests CI
+on:
+  push:
+    branches:
+      - master
+      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
+  pull_request:
+    branches:
+      - master
+      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
+
+concurrency:
+  group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || 
github.head_ref || github.ref }}'
+  cancel-in-progress: true
+
+jobs:
+  build:
+    strategy:
+      fail-fast: false
+      matrix:
+        jdk: [ '8', '11', '17' ]
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout branch
+        uses: actions/checkout@v3
+      - name: Cache Maven m2 repository
+        id: maven
+        uses: actions/cache@v3
+        with:
+          path: ~/.m2/repository
+          key: maven-${{ runner.os }}-${{ matrix.jdk }}-${{ 
hashFiles('**/pom.xml') }}
+          restore-keys: |
+            maven-${{ runner.os }}-${{ matrix.jdk }}
+      - name: Cache targets
+        id: target
+        uses: actions/cache@v3
+        with:
+          path: |
+            ./target
+            ./extendedset/target
+            ./hll/target
+            ./processing/target
+            ./server/target
+            ./indexing-hadoop/target
+            ./web-console/target
+            ./indexing-service/target
+            ./services/target
+            ./benchmarks/target
+            ./distribution/target
+            ./integration-tests/target

Review Comment:
   The above is a very incomplete list! We're missing all the extensions, plus 
the new ITs, plus sql, etc. Is the goal to cache all the target directories? Is 
there a way to use a wildcard? In Druid, they should all be in `*/target` or 
`*/*/*/target`.
   
   If a target directory is not listed here, is it not cached? And, if it is 
not cached, it will be rebuilt by Maven as needed on each build step?
   
   Might be worth carefully looking at a build to see if we are, in fact, using 
the cache or if Maven is doing an unnecessary rebuild.



##########
.github/workflows/reusable-standard-its.yml:
##########
@@ -0,0 +1,91 @@
+# 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.
+
+name: Revised Integration Tests shared workflow
+on:
+  workflow_call:
+    inputs:
+      runtime_jdk:
+        description: 'Which JDK version to use at runtime'
+        required: true
+        type: string
+      use_indexer:
+        description: 'Which indexer to use'
+        required: true
+        type: string
+      override_config_path:
+        required: false
+        type: string
+      testing_groups:
+        required: true
+        type: string
+      build_jdk:
+        description: 'Which jdk version was used to build'
+        required: true
+        type: string
+      mysql_driver:
+        description: MySQL driver to use
+        required: false
+        type: string
+        default: com.mysql.jdbc.Driver
+
+env:
+  MVN: mvn --no-snapshot-updates
+  MAVEN_SKIP: -P skip-static-checks -Dweb.console.skip=true 
-Dmaven.javadoc.skip=true
+  MAVEN_SKIP_TESTS: -P skip-tests
+  DOCKER_IP: 127.0.0.1  # for integration tests
+  MYSQL_DRIVER_CLASSNAME: ${{ inputs.mysql_driver }}
+
+jobs:
+  test:
+    runs-on: ubuntu-22.04
+    steps:
+      - name: Checkout branch
+        uses: actions/checkout@v3
+      - name: Setup java
+        uses: actions/setup-java@v3
+        with:
+          distribution: 'zulu'
+          java-version: ${{ inputs.runtime_jdk }}
+      - name: Restore Maven repository
+        uses: actions/cache/restore@v3
+        with:
+         path: ~/.m2/repository
+         key: maven-${{ runner.os }}-${{ inputs.build_jdk }}-${{ 
hashFiles('**/pom.xml') }}
+      - name: Restore targets
+        uses: actions/cache/restore@v3
+        with:
+          path: |
+            ./target
+            ./extendedset/target
+            ./hll/target
+            ./processing/target
+            ./server/target
+            ./indexing-hadoop/target
+            ./web-console/target
+            ./indexing-service/target
+            ./services/target
+            ./benchmarks/target
+            ./distribution/target
+            ./integration-tests/target
+          key: maven-${{ runner.os }}-${{ inputs.build_jdk }}-targets-${{ 
hashFiles('**/pom.xml') }}
+      - name: Run IT
+        env:
+          MYSQL_DRIVER_CLASSNAME: ${{ inputs.mysql_driver }}
+        run: |
+          # Debug echo
+          echo "Mysql driver: ${MYSQL_DRIVER_CLASSNAME}"
+          echo "${MVN} verify -pl integration-tests -P integration-tests ${{ 
inputs.testing_groups }} -Djvm.runtime=${{ inputs.runtime_jdk }} 
-Dit.indexer=${{ inputs.use_indexer }} ${MAVEN_SKIP} -Doverride.config.path=${{ 
inputs.override_config_path }}"
+          ${MVN} verify -pl integration-tests -P integration-tests ${{ 
inputs.testing_groups }} -Djvm.runtime=${{ inputs.runtime_jdk }} 
-Dit.indexer=${{ inputs.use_indexer }} ${MAVEN_SKIP} -Doverride.config.path=${{ 
inputs.override_config_path }}

Review Comment:
   Just so we are all on the same page. This old-IT command line will:
   
   * Build the entire source tree (using different Maven options)
   * Build up a distribution in its own odd way.
   * Create a docker image, which pulls down Kafka, MySQL and ZK.
   * Start the cluster
   * Run the test
   * Stop the cluster
   * Throw away everything we created above
   
   We don't want to do all this heavy work any more than necessary. (This is 
why we want to convert the old ITs to the new form.)



##########
.github/workflows/it-ex.yml:
##########
@@ -0,0 +1,38 @@
+# 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.
+
+on:
+  workflow_call:
+    inputs:
+      druid_it_image_name:
+        required: true
+        type: string
+jobs:
+  it:
+    strategy:
+      fail-fast: false
+      matrix:
+        #jdk: [8, 11, 17]
+        jdk: [8]
+        it: [HighAvailability, MultiStageQuery, Catalog, BatchIndex]
+        #indexer: [indexer, middleManager]
+        indexer: [middleManager]
+    uses: 
tejaswini-imply/druid/.github/workflows/reusable-revised-its.yml@update_uts_gha
+    with:
+      build_jdk: ${{ matrix.jdk }}
+      runtime_jdk: ${{ matrix.jdk }}
+      use_indexer: ${{ matrix.indexer }}

Review Comment:
   It turns out some of the ITs can use indexer, some can use MM, and some can 
use both. How should we handle tests that can use one but not the other?
   
   One option is to add conditionals here, but that would be messy and hard to 
maintain.
   
   Another option is to add conditionals in the `it.sh` script. Not quite as 
messy, but still hard to maintain.
   
   A third option is to add a file, `indexers.txt`, to each test category. It 
would contain the `USE_INDEXER` values that each test supports. `it.sh` would 
check the file against the `USE_INDEXER` variable and would skip (succeed) 
tests that don't support the selected indexer maybe with an output of 'Indexer 
type "indexer" not supported by test category "MyTest"'.
   
   Thoughts?



##########
.github/workflows/unit-and-integration-tests-unified.yml:
##########
@@ -0,0 +1,160 @@
+# 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.
+
+name: Unit & Integration tests CI
+on:
+  push:
+    branches:
+      - master
+      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
+  pull_request:
+    branches:
+      - master
+      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
+
+concurrency:
+  group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || 
github.head_ref || github.ref }}'
+  cancel-in-progress: true
+
+jobs:
+  build:
+    strategy:
+      fail-fast: false
+      matrix:
+        jdk: [ '8', '11', '17' ]
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout branch
+        uses: actions/checkout@v3
+      - name: Cache Maven m2 repository
+        id: maven
+        uses: actions/cache@v3
+        with:
+          path: ~/.m2/repository
+          key: maven-${{ runner.os }}-${{ matrix.jdk }}-${{ 
hashFiles('**/pom.xml') }}
+          restore-keys: |
+            maven-${{ runner.os }}-${{ matrix.jdk }}
+      - name: Cache targets
+        id: target
+        uses: actions/cache@v3
+        with:
+          path: |
+            ./target
+            ./extendedset/target
+            ./hll/target
+            ./processing/target
+            ./server/target
+            ./indexing-hadoop/target
+            ./web-console/target
+            ./indexing-service/target
+            ./services/target
+            ./benchmarks/target
+            ./distribution/target
+            ./integration-tests/target
+          key: maven-${{ runner.os }}-${{ matrix.jdk }}-targets-${{ 
hashFiles('**/pom.xml') }}
+          restore-keys: |
+            maven-${{ runner.os }}-${{ matrix.jdk }}-targets
+      - name: Cache image
+        id: docker_container
+        uses: actions/cache@v3
+        with:
+          key: druid-container-jdk${{ matrix.jdk }}.tar.gz-${{ 
hashFiles('**/pom.xml') }}
+          path: |
+            ./druid-container-jdk${{ matrix.jdk }}.tar.gz
+            ./integration-tests-ex/image/target/env.sh
+          restore-keys: |
+            druid-container-jdk${{ matrix.jdk }}.tar.gz-
+      - name: Load docker image
+        if: steps.docker_container.outputs.cache-hit == 'true'
+        run: |
+          docker load --input druid-container-jdk${{ matrix.jdk }}.tar.gz
+          docker images
+      - name: Setup java
+        if: (steps.docker_container.outputs.cache-hit != 'true' || 
steps.target.outputs.cache-hit != 'true')
+        uses: actions/setup-java@v3
+        with:
+          distribution: 'zulu'
+          java-version: ${{ matrix.jdk }}
+      - name: Maven build
+        if: (steps.maven.outputs.cache-hit != 'true' || 
steps.target.outputs.cache-hit != 'true')
+        id: maven_build
+        run: |
+          ./it.sh ci
+      - name: Container build
+        if: (steps.docker_container.outputs.cache-hit != 'true' || 
steps.maven.outputs.cache-hit != 'true' || steps.target.outputs.cache-hit != 
'true')
+        run: |
+          ./it.sh image
+          source ./integration-tests-ex/image/target/env.sh
+          docker tag $DRUID_IT_IMAGE_NAME $DRUID_IT_IMAGE_NAME-jdk${{ 
matrix.jdk }}
+      - name: Set docker image name
+        id: docker_image_name
+        run: |
+          source ./integration-tests-ex/image/target/env.sh
+          echo "DRUID_IT_IMAGE_NAME=$(echo $DRUID_IT_IMAGE_NAME | sed 
's/\./_/g; s/\//_/g; s/\:/-/g')" >> $GITHUB_OUTPUT
+      #- name: Upload image
+      #  if: steps.docker_env.outputs.cache-hit != 'true'
+      #  uses: ishworkh/docker-image-artifact-upload@v1
+      #  with:
+      #    image: "${{ steps.docker_image_name.outputs.DRUID_IT_IMAGE_NAME 
}}-jdk${{ matrix.jdk }}"
+      #    retention_days: "2"
+      - name: Save docker container to archive
+        run: |
+          source ./integration-tests-ex/image/target/env.sh
+          echo $DRUID_IT_IMAGE_NAME
+          docker save "$DRUID_IT_IMAGE_NAME" | gzip > druid-container-jdk${{ 
matrix.jdk }}.tar.gz
+      #- name: Upload image
+      #  if: steps.docker_env.outputs.cache-hit != 'true'
+      #  uses: actions/upload-artifact@v3
+      #  with:
+      #    name: ${{ steps.docker_image_name.outputs.DRUID_IT_IMAGE_NAME 
}}-jdk${{ matrix.jdk }}.tar.gz
+      #    path: ./${{ steps.docker_image_name.outputs.DRUID_IT_IMAGE_NAME 
}}-jdk${{ matrix.jdk }}.tar.gz
+      #    retention-days: "1"
+    outputs:
+      DRUID_IT_IMAGE_NAME: ${{ 
steps.docker_image_name.outputs.DRUID_IT_IMAGE_NAME }}
+
+  unit-tests:
+    strategy:
+      fail-fast: false
+      matrix:
+        sql_compatibility: [ false, true ]
+    name: jdk8, sql-compat=${{ matrix.sql_compatibility }}
+    uses: 
tejaswini-imply/druid/.github/workflows/reusable-unit-tests.yml@update_uts_gha

Review Comment:
   Will these names eventually switch to a Druid user name?



##########
.github/workflows/reusable-standard-its.yml:
##########
@@ -0,0 +1,91 @@
+# 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.
+
+name: Revised Integration Tests shared workflow
+on:
+  workflow_call:
+    inputs:
+      runtime_jdk:
+        description: 'Which JDK version to use at runtime'
+        required: true
+        type: string
+      use_indexer:
+        description: 'Which indexer to use'
+        required: true
+        type: string
+      override_config_path:
+        required: false
+        type: string
+      testing_groups:
+        required: true
+        type: string
+      build_jdk:
+        description: 'Which jdk version was used to build'
+        required: true
+        type: string
+      mysql_driver:
+        description: MySQL driver to use
+        required: false
+        type: string
+        default: com.mysql.jdbc.Driver
+
+env:
+  MVN: mvn --no-snapshot-updates
+  MAVEN_SKIP: -P skip-static-checks -Dweb.console.skip=true 
-Dmaven.javadoc.skip=true
+  MAVEN_SKIP_TESTS: -P skip-tests
+  DOCKER_IP: 127.0.0.1  # for integration tests
+  MYSQL_DRIVER_CLASSNAME: ${{ inputs.mysql_driver }}

Review Comment:
   Actually, we don't need to test the MySQL driver for all tests. Pick one 
metadata-heavy test and run just that one with the other driver. We really 
won't learn much by running all tests with both drivers. Any problem will show 
up pretty quickly in one DB-heavy test.



##########
.github/workflows/reusable-standard-its.yml:
##########
@@ -0,0 +1,91 @@
+# 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.
+
+name: Revised Integration Tests shared workflow
+on:
+  workflow_call:
+    inputs:
+      runtime_jdk:
+        description: 'Which JDK version to use at runtime'
+        required: true
+        type: string
+      use_indexer:
+        description: 'Which indexer to use'
+        required: true
+        type: string
+      override_config_path:
+        required: false
+        type: string
+      testing_groups:
+        required: true
+        type: string
+      build_jdk:
+        description: 'Which jdk version was used to build'
+        required: true
+        type: string
+      mysql_driver:
+        description: MySQL driver to use
+        required: false
+        type: string
+        default: com.mysql.jdbc.Driver
+
+env:
+  MVN: mvn --no-snapshot-updates
+  MAVEN_SKIP: -P skip-static-checks -Dweb.console.skip=true 
-Dmaven.javadoc.skip=true
+  MAVEN_SKIP_TESTS: -P skip-tests
+  DOCKER_IP: 127.0.0.1  # for integration tests
+  MYSQL_DRIVER_CLASSNAME: ${{ inputs.mysql_driver }}

Review Comment:
   `DOCKER_IP` is available for the new ITs also. It defaults to `127.0.0.1` (I 
believe, would have to check). Should we set it for the new ITs as well?
   
   The new ITs either already include both MySQL drivers, or we could add the 
missing one. Should we use both for the new ITs?



##########
.github/workflows/reusable-unit-tests.yml:
##########
@@ -11,22 +11,20 @@
 # 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.
+# limitations under the License
 
-name: (openjdk8) Unit Tests CI
 on:
-  push:
-    branches:
-      - master
-      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
-  pull_request:
-    branches:
-      - master
-      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
-
-concurrency:
-  group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || 
github.head_ref || github.ref }}'
-  cancel-in-progress: true
+  workflow_call:

Review Comment:
   On each of these scripts, would be great to add comments explaining the 
purpose and how it will be used. By now, this is probably intuitive to you, as 
the author. It is a bit less clear to us reviewers. And, I'll bet that in six 
months, it won't even be as clear to you...



##########
.github/workflows/unit-and-integration-tests-unified.yml:
##########
@@ -0,0 +1,160 @@
+# 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.
+
+name: Unit & Integration tests CI
+on:
+  push:
+    branches:
+      - master
+      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
+  pull_request:
+    branches:
+      - master
+      - /^\d+\.\d+\.\d+(-\S*)?$/ # release branches
+
+concurrency:
+  group: '${{ github.workflow }} @ ${{ github.event.pull_request.head.label || 
github.head_ref || github.ref }}'
+  cancel-in-progress: true
+
+jobs:
+  build:
+    strategy:
+      fail-fast: false
+      matrix:
+        jdk: [ '8', '11', '17' ]
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout branch
+        uses: actions/checkout@v3
+      - name: Cache Maven m2 repository
+        id: maven
+        uses: actions/cache@v3
+        with:
+          path: ~/.m2/repository
+          key: maven-${{ runner.os }}-${{ matrix.jdk }}-${{ 
hashFiles('**/pom.xml') }}
+          restore-keys: |
+            maven-${{ runner.os }}-${{ matrix.jdk }}
+      - name: Cache targets
+        id: target
+        uses: actions/cache@v3
+        with:
+          path: |
+            ./target
+            ./extendedset/target
+            ./hll/target
+            ./processing/target
+            ./server/target
+            ./indexing-hadoop/target
+            ./web-console/target
+            ./indexing-service/target
+            ./services/target
+            ./benchmarks/target
+            ./distribution/target
+            ./integration-tests/target
+          key: maven-${{ runner.os }}-${{ matrix.jdk }}-targets-${{ 
hashFiles('**/pom.xml') }}
+          restore-keys: |
+            maven-${{ runner.os }}-${{ matrix.jdk }}-targets
+      - name: Cache image
+        id: docker_container
+        uses: actions/cache@v3
+        with:
+          key: druid-container-jdk${{ matrix.jdk }}.tar.gz-${{ 
hashFiles('**/pom.xml') }}
+          path: |
+            ./druid-container-jdk${{ matrix.jdk }}.tar.gz
+            ./integration-tests-ex/image/target/env.sh
+          restore-keys: |
+            druid-container-jdk${{ matrix.jdk }}.tar.gz-
+      - name: Load docker image
+        if: steps.docker_container.outputs.cache-hit == 'true'
+        run: |
+          docker load --input druid-container-jdk${{ matrix.jdk }}.tar.gz
+          docker images
+      - name: Setup java
+        if: (steps.docker_container.outputs.cache-hit != 'true' || 
steps.target.outputs.cache-hit != 'true')
+        uses: actions/setup-java@v3
+        with:
+          distribution: 'zulu'
+          java-version: ${{ matrix.jdk }}
+      - name: Maven build
+        if: (steps.maven.outputs.cache-hit != 'true' || 
steps.target.outputs.cache-hit != 'true')
+        id: maven_build
+        run: |
+          ./it.sh ci
+      - name: Container build
+        if: (steps.docker_container.outputs.cache-hit != 'true' || 
steps.maven.outputs.cache-hit != 'true' || steps.target.outputs.cache-hit != 
'true')
+        run: |
+          ./it.sh image
+          source ./integration-tests-ex/image/target/env.sh

Review Comment:
   The `it.sh` script picks a default image name. That's handy when run on a 
desktop. For GHA, it might make sense to set the image name in GHA in 
`DRUID_IT_IMAGE_NAME` then `it.sh image` will pick up and use that name. Since 
it does, you don't then need to recover the name from `env.sh`.
   
   To be clear, in the old Travis build, `DRUID_IT_IMAGE_NAME` as passed into 
`it.sh image` which then writes the value into `env.sh` so that tests can find 
it without having to be told the name again.



-- 
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]


Reply via email to