andygrove commented on PR #5489:
URL:
https://github.com/apache/datafusion-comet/pull/5489#issuecomment-5441357137
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
Thanks for chasing this. Transient dependency-download failures are a real
tax on everyone's PRs and the two linked failures make the case clearly.
Factoring the Maven bootstrap into a shared composite action is overdue, and
the `cache-path.sh` fix for `EACCES` on hosted runners is a good catch:
globbing `/root` from a non-root job was always going to bite eventually.
I do have concerns, mostly about scope and about one cache key.
**This is several independent changes in one PR**
As far as I can tell there are at least four separable pieces here: the
shared `setup-maven` action plus cache-path fix, the `retry-download.sh`
wrapper, the SBT dependency pre-resolution and its new cache, and the Delta
gate log preservation. CI changes are the hardest kind to review because most
of the behavior only shows up in production, and if any one of these regresses,
a combined PR means reverting all four. Would you be willing to split at least
the SBT pre-resolution out? That one has the most new moving parts and the
least connection to the download-failure problem in the title.
**The SBT cache key mixes Comet and Spark poms**
`Restore SBT dependency cache` keys on `hashFiles('**/pom.xml',
'!**/target/**', 'apache-spark/project/build.properties',
'apache-spark/project/*.sbt', 'apache-spark/project/*.scala')`. The
`**/pom.xml` glob is evaluated against the whole workspace, which at that point
includes the cloned and patched `apache-spark` checkout. The saving job clones
Spark itself. The test shards pass `skip-spark-clone: true` and get
`apache-spark/` from a pre-staged artifact.
If those two paths produce even slightly different file sets under
`apache-spark`, the hash differs and the restore falls through to the
`restore-keys` prefix. That still works, but it means the exact-key path never
hits and you get whatever the most recent prefix match happens to be. Have you
confirmed the primary key actually matches between the saving job and a
restoring shard? If not, it would be more predictable to key on the Spark
version plus the specific SBT build files and drop the wide pom glob, since
Comet's own poms do not determine Spark's SBT dependency graph.
**`setup-maven` is not idempotent in `MAVEN_OPTS`**
The `Configure Maven transfer retries` step appends the
`aether.connector.http.retryHandler.*` properties to whatever `MAVEN_OPTS`
already holds and writes the result to `$GITHUB_ENV`. If a job ever ends up
calling `setup-maven` twice, which is easy to do now that it is a shared action
invoked from several places, the properties accumulate. It is harmless today
because later `-D` wins, but it grows the command line and makes the logs
confusing. Could the step skip appending when the property is already present?
**`cache-path.sh` word-splits `MAVEN_OPTS`**
`read -r -a java_arguments <<< "$java_options"` splits on whitespace with no
quote handling. If anyone ever puts a quoted value containing a space into
`MAVEN_OPTS`, for example an `-XX:ErrorFile` under a path with a space, the
`java -XshowSettings` probe fails and the whole action errors out at a point
that has nothing to do with the actual problem. Since only a handful of
properties are needed to make the probe representative, would it be simpler to
run the probe with a fixed minimal set of options rather than replaying
whatever `MAVEN_OPTS` contains?
**The transient-error regex matches anywhere in the log**
`retry-download.sh` greps the entire combined output for the transient
patterns. For a long `sbt update` over Spark, the log is large and includes
artifact names and URLs. A permanent failure that happens to occur in the same
run as an earlier recovered transient message would be retried three more times
before failing, turning a fast failure into a slow one. Is it worth restricting
the match to the tail of the log, or to lines that also indicate the command's
own failure? The current behavior is safe in the sense that it never turns a
failure into a pass, but the cost is real when it goes wrong.
**One question on the preflight tests**
`test-download-retry.py` and `test-delta-gate.py` add 600 lines that now run
on every preflight. How long do they take? The preflight job is on the critical
path for every PR, and 24 regression cases including real Maven requests
against a local HTTP server sounds like it could be a minute or more. If it is
significant, gating them on changes to `dev/ci/**` and the actions directory
would keep the common case fast.
--
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]
