voonhous commented on code in PR #19986:
URL: https://github.com/apache/hudi/pull/19986#discussion_r4056843994
##########
.github/workflows/scheduled_workflow.yml:
##########
@@ -34,11 +34,12 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Delete Cache
+ # Spark archives must survive the purge so integration-tests can reuse
them.
run: |
gh extension install actions/gh-actions-cache
echo "Fetching list of cache key"
- cacheKeysForPR=$(gh actions-cache list -R $REPO -L 100 | cut -f 1 )
+ cacheKeysForPR=$(gh actions-cache list -R $REPO -L 100 | cut -f 1 |
grep -v '^spark-archive-v1-' || true)
Review Comment:
**major:** This filter pins the literal `v1`, but the key it has to match
lives in another workflow (`bot.yml:1518`). Bumping the key to
`spark-archive-v2-` to invalidate a stale entry silently re-arms the purge, and
the cache then misses forever with no error in any log.
Could we anchor on the version-independent prefix, and add a one-line
comment at `bot.yml:1518` pointing here?
```suggestion
cacheKeysForPR=$(gh actions-cache list -R $REPO -L 100 | cut -f 1
| grep -v '^spark-archive-' || true)
```
##########
.github/workflows/scheduled_workflow.yml:
##########
@@ -34,11 +34,12 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Delete Cache
+ # Spark archives must survive the purge so integration-tests can reuse
them.
Review Comment:
**major:** The purge was a 2024 stopgap for oversized `setup-java` maven
caches (HUDI-8180, #11904: "the caches are bad and prevent the actions from
ever being successful"). That cause was removed in #12053, but the job stayed,
and #17738 later put `cache: maven` back on 13 `bot.yml` jobs.
`hudi_trino_ci.yml:157`'s `trino-m2-v2-` cache is dead for the same reason,
costing a 2m25s Trino rebuild per run.
Would it be worth fixing the purge once, with a filter that covers
`trino-m2-v2-` too, rather than adding one prefix per PR?
##########
.github/workflows/bot.yml:
##########
@@ -1509,43 +1509,60 @@ jobs:
SCALA_PROFILE: '-Dscala-2.12 -Dscala.binary.version=2.12'
run:
mvn test $SCALA_PROFILE -D"$SPARK_PROFILE" -Pintegration-tests
-DskipUTs=false -DskipITs=true -pl hudi-integ-test $MVN_ARGS -Djacoco.skip=false
+ - name: Restore Spark archive
+ if: needs.changes.outputs.relevant == 'true'
+ id: spark-archive
+ uses: actions/cache/restore@v4
+ with:
+ path: ~/spark-archives/${{ matrix.sparkArchive }}
+ key: spark-archive-v1-${{ runner.os }}-${{ matrix.sparkArchive }}
- name: 'IT'
if: needs.changes.outputs.relevant == 'true'
env:
SPARK_PROFILE: ${{ matrix.sparkProfile }}
SPARK_ARCHIVE: ${{ matrix.sparkArchive }}
SCALA_PROFILE: '-Dscala-2.12 -Dscala.binary.version=2.12'
+ SPARK_ARCHIVE_CACHE_HIT: ${{ steps.spark-archive.outputs.cache-hit }}
run: |
# dlcdn only carries the current release of each line; fall back to
# the archive for older pins (#19883). Plain --retry, not
# --retry-all-errors, so a 404 on the CDN falls through immediately.
# --speed-limit is a dead-connection detector, not a slowness one:
# --retry truncates the output back to byte 0, so a floor set near
# the archive's real throughput re-downloads 382MB per abort.
- DEST="$GITHUB_WORKSPACE/$SPARK_ARCHIVE"
+ DEST="$HOME/spark-archives/$SPARK_ARCHIVE"
downloaded=false
- for base in https://dlcdn.apache.org/spark
https://archive.apache.org/dist/spark; do
- echo "Downloading $SPARK_ARCHIVE from $base"
- if curl -fL --create-dirs -o "$DEST" \
- --retry 5 --retry-delay 10 \
- --connect-timeout 30 --speed-limit 1000 --speed-time 120 \
- "$base/$SPARK_ARCHIVE"; then
- downloaded=true
- break
+ if [ "$SPARK_ARCHIVE_CACHE_HIT" != 'true' ]; then
+ for base in https://dlcdn.apache.org/spark
https://archive.apache.org/dist/spark; do
+ echo "Downloading $SPARK_ARCHIVE from $base"
+ if curl -fL --create-dirs -o "$DEST" \
+ --retry 5 --retry-delay 10 \
+ --connect-timeout 30 --speed-limit 1000 --speed-time 120 \
+ "$base/$SPARK_ARCHIVE"; then
+ downloaded=true
+ break
+ fi
+ echo "$base did not serve $SPARK_ARCHIVE"
+ rm -f "$DEST"
+ done
+ if [ "$downloaded" != true ]; then
+ echo "ERROR: could not download $SPARK_ARCHIVE from any source"
+ exit 1
fi
- echo "$base did not serve $SPARK_ARCHIVE"
- rm -f "$DEST"
- done
- if [ "$downloaded" != true ]; then
- echo "ERROR: could not download $SPARK_ARCHIVE from any source"
- exit 1
+ else
+ echo "Restored $SPARK_ARCHIVE from the actions cache"
fi
tar -xf "$DEST" -C $GITHUB_WORKSPACE/
mkdir /tmp/spark-events/
SPARK_ARCHIVE_BASENAME=$(basename $SPARK_ARCHIVE)
export SPARK_HOME=$GITHUB_WORKSPACE/${SPARK_ARCHIVE_BASENAME%.*}
- rm -f $GITHUB_WORKSPACE/$SPARK_ARCHIVE
mvn verify $SCALA_PROFILE -D"$SPARK_PROFILE" -Pintegration-tests -pl
!hudi-flink-datasource/hudi-flink $MVN_ARGS -Djacoco.skip=false
+ - name: Save Spark archive
Review Comment:
**minor:** Not blocking. The save sits after `mvn verify`, so what gets
cached is gated on Hudi test health rather than on the tarball being valid.
Would it be worth splitting the download and extract into their own step and
putting `cache/save` immediately after a successful `tar -xf`? That makes the
extract itself the integrity gate on the entry, and decouples seeding from a
red master IT.
##########
.github/workflows/bot.yml:
##########
@@ -1509,43 +1509,60 @@ jobs:
SCALA_PROFILE: '-Dscala-2.12 -Dscala.binary.version=2.12'
run:
mvn test $SCALA_PROFILE -D"$SPARK_PROFILE" -Pintegration-tests
-DskipUTs=false -DskipITs=true -pl hudi-integ-test $MVN_ARGS -Djacoco.skip=false
+ - name: Restore Spark archive
+ if: needs.changes.outputs.relevant == 'true'
+ id: spark-archive
+ uses: actions/cache/restore@v4
+ with:
+ path: ~/spark-archives/${{ matrix.sparkArchive }}
+ key: spark-archive-v1-${{ runner.os }}-${{ matrix.sparkArchive }}
- name: 'IT'
if: needs.changes.outputs.relevant == 'true'
env:
SPARK_PROFILE: ${{ matrix.sparkProfile }}
SPARK_ARCHIVE: ${{ matrix.sparkArchive }}
SCALA_PROFILE: '-Dscala-2.12 -Dscala.binary.version=2.12'
+ SPARK_ARCHIVE_CACHE_HIT: ${{ steps.spark-archive.outputs.cache-hit }}
run: |
# dlcdn only carries the current release of each line; fall back to
# the archive for older pins (#19883). Plain --retry, not
# --retry-all-errors, so a 404 on the CDN falls through immediately.
# --speed-limit is a dead-connection detector, not a slowness one:
# --retry truncates the output back to byte 0, so a floor set near
# the archive's real throughput re-downloads 382MB per abort.
- DEST="$GITHUB_WORKSPACE/$SPARK_ARCHIVE"
+ DEST="$HOME/spark-archives/$SPARK_ARCHIVE"
downloaded=false
- for base in https://dlcdn.apache.org/spark
https://archive.apache.org/dist/spark; do
- echo "Downloading $SPARK_ARCHIVE from $base"
- if curl -fL --create-dirs -o "$DEST" \
- --retry 5 --retry-delay 10 \
- --connect-timeout 30 --speed-limit 1000 --speed-time 120 \
- "$base/$SPARK_ARCHIVE"; then
- downloaded=true
- break
+ if [ "$SPARK_ARCHIVE_CACHE_HIT" != 'true' ]; then
+ for base in https://dlcdn.apache.org/spark
https://archive.apache.org/dist/spark; do
+ echo "Downloading $SPARK_ARCHIVE from $base"
+ if curl -fL --create-dirs -o "$DEST" \
+ --retry 5 --retry-delay 10 \
+ --connect-timeout 30 --speed-limit 1000 --speed-time 120 \
+ "$base/$SPARK_ARCHIVE"; then
+ downloaded=true
+ break
+ fi
+ echo "$base did not serve $SPARK_ARCHIVE"
+ rm -f "$DEST"
+ done
+ if [ "$downloaded" != true ]; then
+ echo "ERROR: could not download $SPARK_ARCHIVE from any source"
+ exit 1
fi
- echo "$base did not serve $SPARK_ARCHIVE"
- rm -f "$DEST"
- done
- if [ "$downloaded" != true ]; then
- echo "ERROR: could not download $SPARK_ARCHIVE from any source"
- exit 1
+ else
+ echo "Restored $SPARK_ARCHIVE from the actions cache"
fi
tar -xf "$DEST" -C $GITHUB_WORKSPACE/
mkdir /tmp/spark-events/
SPARK_ARCHIVE_BASENAME=$(basename $SPARK_ARCHIVE)
export SPARK_HOME=$GITHUB_WORKSPACE/${SPARK_ARCHIVE_BASENAME%.*}
- rm -f $GITHUB_WORKSPACE/$SPARK_ARCHIVE
mvn verify $SCALA_PROFILE -D"$SPARK_PROFILE" -Pintegration-tests -pl
!hudi-flink-datasource/hudi-flink $MVN_ARGS -Djacoco.skip=false
+ - name: Save Spark archive
+ if: needs.changes.outputs.relevant == 'true' && github.event_name ==
'push' && github.ref == 'refs/heads/master' &&
steps.spark-archive.outputs.cache-hit != 'true'
Review Comment:
**major:** No pull_request run can exercise either new step, so the green
check here proves nothing about them. `*.yml` is not "relevant" to the
`changes` filter, so on this PR's own run (35225384777) the integration-tests
job finished in 5 seconds with all 14 steps skipped, including `Restore Spark
archive` and `Save Spark archive`. This gate then limits the save to master
pushes.
Could we validate on a temporary branch with this gate widened and a non-yml
file touched, before it first runs for real on master?
##########
.github/workflows/bot.yml:
##########
@@ -1509,43 +1509,60 @@ jobs:
SCALA_PROFILE: '-Dscala-2.12 -Dscala.binary.version=2.12'
run:
mvn test $SCALA_PROFILE -D"$SPARK_PROFILE" -Pintegration-tests
-DskipUTs=false -DskipITs=true -pl hudi-integ-test $MVN_ARGS -Djacoco.skip=false
+ - name: Restore Spark archive
+ if: needs.changes.outputs.relevant == 'true'
+ id: spark-archive
+ uses: actions/cache/restore@v4
+ with:
+ path: ~/spark-archives/${{ matrix.sparkArchive }}
+ key: spark-archive-v1-${{ runner.os }}-${{ matrix.sparkArchive }}
- name: 'IT'
if: needs.changes.outputs.relevant == 'true'
env:
SPARK_PROFILE: ${{ matrix.sparkProfile }}
SPARK_ARCHIVE: ${{ matrix.sparkArchive }}
SCALA_PROFILE: '-Dscala-2.12 -Dscala.binary.version=2.12'
+ SPARK_ARCHIVE_CACHE_HIT: ${{ steps.spark-archive.outputs.cache-hit }}
run: |
# dlcdn only carries the current release of each line; fall back to
# the archive for older pins (#19883). Plain --retry, not
# --retry-all-errors, so a 404 on the CDN falls through immediately.
# --speed-limit is a dead-connection detector, not a slowness one:
# --retry truncates the output back to byte 0, so a floor set near
# the archive's real throughput re-downloads 382MB per abort.
- DEST="$GITHUB_WORKSPACE/$SPARK_ARCHIVE"
+ DEST="$HOME/spark-archives/$SPARK_ARCHIVE"
downloaded=false
- for base in https://dlcdn.apache.org/spark
https://archive.apache.org/dist/spark; do
- echo "Downloading $SPARK_ARCHIVE from $base"
- if curl -fL --create-dirs -o "$DEST" \
- --retry 5 --retry-delay 10 \
- --connect-timeout 30 --speed-limit 1000 --speed-time 120 \
- "$base/$SPARK_ARCHIVE"; then
- downloaded=true
- break
+ if [ "$SPARK_ARCHIVE_CACHE_HIT" != 'true' ]; then
+ for base in https://dlcdn.apache.org/spark
https://archive.apache.org/dist/spark; do
+ echo "Downloading $SPARK_ARCHIVE from $base"
+ if curl -fL --create-dirs -o "$DEST" \
+ --retry 5 --retry-delay 10 \
+ --connect-timeout 30 --speed-limit 1000 --speed-time 120 \
+ "$base/$SPARK_ARCHIVE"; then
+ downloaded=true
+ break
+ fi
+ echo "$base did not serve $SPARK_ARCHIVE"
+ rm -f "$DEST"
+ done
+ if [ "$downloaded" != true ]; then
+ echo "ERROR: could not download $SPARK_ARCHIVE from any source"
+ exit 1
fi
- echo "$base did not serve $SPARK_ARCHIVE"
- rm -f "$DEST"
- done
- if [ "$downloaded" != true ]; then
- echo "ERROR: could not download $SPARK_ARCHIVE from any source"
- exit 1
+ else
+ echo "Restored $SPARK_ARCHIVE from the actions cache"
fi
tar -xf "$DEST" -C $GITHUB_WORKSPACE/
Review Comment:
**nit:** Feel free to ignore. #19884 left checksum verification out of scope
when the tarball was re-fetched every run; that is weaker now that one entry is
retained indefinitely and extracted into `$SPARK_HOME`. Apache publishes a
`.sha512` beside every archive.
Would a `sha512sum -c` after both the restore and the download paths be
worth the two lines?
##########
.github/workflows/bot.yml:
##########
@@ -1509,43 +1509,60 @@ jobs:
SCALA_PROFILE: '-Dscala-2.12 -Dscala.binary.version=2.12'
run:
mvn test $SCALA_PROFILE -D"$SPARK_PROFILE" -Pintegration-tests
-DskipUTs=false -DskipITs=true -pl hudi-integ-test $MVN_ARGS -Djacoco.skip=false
+ - name: Restore Spark archive
Review Comment:
**minor:** Not blocking, but the "up to 68 minutes" in Impact is a
pre-#19884 number. Measured on six master runs since dlcdn landed, the download
takes 1-18s (median ~11s) against a ~36 minute job, and a 382MB restore is
~4.5s extrapolated from a 1435MB restore in ~17s (job 105516288805). Net is
roughly 6-14s.
Could we restate Impact as removing a 382MB egress and its stale-pin tail
risk, rather than as minutes?
--
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]
