Locate and download Prism binary #31796
Conversation
damondouglas
force-pushed
the
prism-java-runner-wrapper-resolver
branch
from
29ea609
to
e0a355f
Compare
damondouglas
changed the title
|
Assigning reviewers. If you would like to opt out of this review, comment R: @damccorm added as fallback since no labels match configuration Available commands:
The PR bot will only process comments in the main thread (not review comments). |
runners/prism/java/src/main/java/org/apache/beam/runners/prism/PrismLocator.java
Outdated
Show resolved
Hide resolved
runners/prism/java/src/main/java/org/apache/beam/runners/prism/PrismLocator.java
Show resolved
Hide resolved
runners/prism/java/src/main/java/org/apache/beam/runners/prism/PrismLocator.java
Outdated
Show resolved
Hide resolved
| private String resolve(URL from, Path to) throws IOException { | ||
| if (from.toString().startsWith(GITHUB_DOWNLOAD_PREFIX)) { | ||
| URL shaSumReference = new URL(from + "." + SHA512_EXT); | ||
| validateShaSum512(shaSumReference, from); |
There was a problem hiding this comment.
What does this validation get us actually? What vector are we protecting against here?
There was a problem hiding this comment.
Validates download happened correctly.
There was a problem hiding this comment.
Not super familiar with the pattern being used here, so this could be me not understanding how this ordering works, but doesn't the actual download happen after this in downloadFn? The way I'm reading this, validateShaSum512 verifies that the remote files match by downloading/comparing, but then downloadFn redownloads it
There was a problem hiding this comment.
It is still "downloading" as part of the sha verification. Instead of storing the bytes on a temporary file, it just validates from the bytes directly.
There was a problem hiding this comment.
Because of the likelihood of downloading a zip or not, it was just easier to keep it separate. It's not that big a file so it doesn't matter. It would be ugly to have to store the bytes then shove it in a file and figure out whether or not to do that when its a zip or not as a source. Honestly, I'm tempted to just remove this validateShaSum method. The goal of this whole PR is just to download the thing.
There was a problem hiding this comment.
My vote is to not do the sha validation at this stage. It would only apply when downloading GitHub. But we aren't making it consistent this release.
There was a problem hiding this comment.
Removed it. Thank you @lostluck
runners/prism/java/src/test/java/org/apache/beam/runners/prism/PrismLocatorTest.java
Show resolved
Hide resolved
runners/prism/java/src/main/java/org/apache/beam/runners/prism/PrismLocator.java
Show resolved
Hide resolved
* Stage PrismRunner implementation and dependencies * Locate and download Prism binary * Sync with head * Remove redundant check * Remove sha verification; delete files in test setup * Remove destination dir; check exists * Add tests for 404 and tag prefix
This PR closes #31402 with a support class that locates, downloads, and prepares the Prism binary.
cc: @lostluck
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.