kgyrtkirk commented on code in PR #20734: URL: https://github.com/apache/druid/pull/20734#discussion_r4398288556
########## .github/scripts/pull_docker_images: ########## @@ -0,0 +1,31 @@ +#!/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. + +# Pulls every Docker image declared via DruidCI.cacheDockerImage("<image>") in Java sources. + +set -eu + +CALL='DruidCI\.cacheDockerImage\(' + +if git grep -nE "$CALL" -- '*.java' | grep -vE "$CALL\"[^\"]+\"\)"; then + echo "::error ::DruidCI.cacheDockerImage must be called with a string literal" + exit 1 +fi + +for image in $(git grep -hoE "$CALL\"[^\"]+\"" -- '*.java' | grep -oE '"[^"]+"' | tr -d '"' | sort -u); do + docker pull --quiet "$image" || echo "::warning ::docker pull $image failed" Review Comment: let me reply with my AI's response; but the key point is: "The retry rescued **0** jobs." # ๐งน Removing the CI retry loops from `worker.yml` ## ๐ Summary `worker.yml` on master has two retry loops: one for parent POM resolution (lines 77-91) and one for docker pull (lines 109-128). Data from the last 64 master push commits (up to `434de02b`) shows that both rarely fire. Neither has changed the outcome of a job. They add complexity without measured benefit, so this PR removes them. ## ๐ฌ Method - `ci-stability --repo apache/druid --branch master --commits 64 --logs` downloaded 1338 job logs, passing jobs included. - Only lines with `##[warning]` were counted as firings. Lines with the step's script echoed back were ignored. ## ๐ฆ Parent POM resolution retry 1022 jobs ran the step. 1020 succeeded on the first attempt. The retry fired in 2 jobs, and both used all 3 attempts and failed. | Commit | Job | Cause | Time in step | Outcome | |---|---|---|---|---| | `c2563d0c` | [QTest 1/4](https://github.com/apache/druid/actions/runs/39535898165/job/119012342540) | attempt 1: `wget: Failed to fetch https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.10.0/apache-maven-3.10.0-bin.zip`; attempts 2-3: `Could not find artifact org.apache:apache:pom:40 in maven.org (https://repo.maven.apache.org/maven2/)` (`absent`) | ~26s | โ failed | | `96431fe6` | [openrewrite](https://github.com/apache/druid/actions/runs/39619665662/job/119289423640) | all 3 attempts: `HTTP Status: 429` from Maven Central | ~6.5 min | โ failed | The retry rescued **0** jobs. Both failures persisted past the 10s backoff, so the retry only made the job fail later. ## ๐ณ Docker pull retry 701 jobs ran the step. 700 had no pull failure. The retry fired once: | Commit | Job | Cause | Outcome | |---|---|---|---| | `587e781a` | [QTest 3/4](https://github.com/apache/druid/actions/runs/39573300818/job/119132560057) | `mcr.microsoft.com/azure-storage/azurite:3.35.0`: `Error response from daemon: received unexpected HTTP status: 503 Service Unavailable` | โ attempt 2 succeeded (~17s); job ok | QTest runs in `quidem-ut` and does not start azurite. Only `embedded-tests` uses that image (`AzureStorageResource`). That one rescued pull therefore made no difference to the job. ## โ๏ธ Why remove - **No effect seen:** across about 1000 jobs in 64 commits, neither retry changed a job's outcome. - **Wrong failure modes:** the failures that did occur (429 rate limits, an `absent` artifact) persisted, and a 10s backoff only postpones the failure. - **Docker pre-pull is best-effort:** if a pre-pull fails, Testcontainers pulls the image when a test needs it. `.github/scripts/pull_docker_images` logs a `::warning` instead of failing the job. - **Floating complexity:** both loops are untested shell code that runs in every job, with no measured benefit. -- 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]
