Repository navigation
Conversation
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR updates PackageUtil.alreadyStaged to check remoteMetadata.lastModifiedMillis() and re-stage artifacts older than 1 day (MAX_STAGED_FILE_AGE), preventing shared staging objects from being deleted mid-job by GCS lifecycle policies (such as the 7-day TTL on gs://temp-storage-for-validates-runner-tests). The implementation and unit test coverage in PackageUtilTest are clean, well-scoped, and backwards-compatible with file systems or mocks that report 0 for lastModifiedMillis.
🔍 General Feedback
- Guarding the age check with
lastModifiedMillis > 0nicely preserves behavior for file system implementations that do not populate modification timestamps as well as existing unit test fixtures. - Re-uploading objects older than 1 day refreshes the GCS object generation and creation timestamp well ahead of the 7-day lifecycle deletion window while maintaining cross-job caching within a 24-hour period.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41258 +/- ##
============================================
+ Coverage 57.74% 58.54% +0.80%
+ Complexity 2634 2291 -343
============================================
Files 1203 1132 -71
Lines 171888 169864 -2024
Branches 2182 1487 -695
============================================
+ Hits 99256 99454 +198
+ Misses 69973 67866 -2107
+ Partials 2659 2544 -115 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
| long lastModifiedMillis = remoteMetadata.lastModifiedMillis(); | ||
| if (lastModifiedMillis > 0 | ||
| && Instant.now().getMillis() - lastModifiedMillis > MAX_STAGED_FILE_AGE.getMillis()) { |
There was a problem hiding this comment.
Instead of checking the if the file age is older than 1 day, can we figure out what the file's TTL is somehow and compare to that?
If not, we should find a different way of handling this (probably staging artifacts with unique names in our tests). The purpose of the staging directory is to reuse previously staged artifacts (even if they were staged a long time ago)
There was a problem hiding this comment.
Good call! GCS object metadata (Objects.get / FileSystems.matchSingleFileSpec) does not expose a TTL or lifecycle expiration timestamp, and bucket lifecycle rules (age: 7 on gs://temp-storage-for-validates-runner-tests) are only available via Buckets.get (storage.buckets.get), whereas production --stagingLocation directories are intended to reuse staged artifacts indefinitely.
Tracing git history showed this race started in PR #40321 (fa34c16167c), which changed TestDataflowRunner from per-job staging (<tempLocation>/staging) to a single static shared directory (<tempRoot>/staging). I reverted the PackageUtil changes and updated TestDataflowRunner.fromOptions in 524a9ee to scope the shared test staging directory under tempRoot by UTC date (staging-<yyyy-MM-dd>), so all tests running on the same day still share and reuse staged JARs while never referencing objects near the 7-day GCS lifecycle deletion threshold.
…age-util-restage-expired
… of modifying PackageUtil
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR resolves recurring Dataflow post-commit failures caused by the 7-day GCS lifecycle deletion rule on gs://temp-storage-for-validates-runner-tests deleting shared staged dependency JARs while jobs are starting up. By scoping the shared TestDataflowRunner staging directory under tempRoot by the UTC date (staging-<yyyy-MM-dd>), tests running on the same day continue to share staged artifacts and in-memory staging cache entries while ensuring staged objects are always fresh (< 24h old).
🔍 General Feedback
- Normalizing
rawStagingLocationviaFileSystems.matchNewDirectorycleanly handles both default per-job paths and explicit--stagingLocation=<tempRoot>/stagingflags (with or without a trailing slash) frombuild.gradle, while preserving custom staging locations. - Evaluating
DAILY_STAGING_DIRonce per JVM load is a good choice, as it keepsstagingLocationconsistent across tests in the same JVM run (preservingGcsStagercache hits andDataflowTestBatchCoordinatorcompatibility keys even if a run crosses UTC midnight). - Unit tests in
TestDataflowRunnerTestthoroughly cover default, unversioned shared, and custom staging location cases.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Note: this PR was auto-generated by tooling. Before moving it out of draft, the author has done a full review of the PR, specifically evaluating it on the following criteria:
Full details of the methodology of generation and review can be found here - https://docs.google.com/document/d/14lj7ECVov4T4EdwmIzNlWrRGE3lLDMOwX5xUMGzaLoc/edit?usp=sharing
If you have concerns with the PR or with the underlying methodology used to generate this, please comment below - they will be addressed by the author not the tooling :)
Summary
Fixes recurring
Workflow failed. Causes: One or more access checks for temp location or staged files failed"failures inPostCommit Java Examples Dataflow V2(run 43313768013) andPostCommit Java ValidatesRunner Dataflow V2 Streaming(run 43025792107).Root Cause
fa34c16167c),TestDataflowRunner.fromOptionsandrunners/google-cloud-dataflow-java/build.gradlewere updated from per-job staging (<tempLocation>/staging) to a static shared staging path (<tempRoot>/staging, i.e.,gs://temp-storage-for-validates-runner-tests/staging) across all runs.gs://temp-storage-for-validates-runner-testsis configured with a 7-day GCS object lifecycleDeleterule (age: 7).PackageUtil.alreadyStagedsees an object staged ~6.99 days ago during job submission and skips re-uploading it, and minutes later GCS's 7-day lifecycle sweeper deletes the object while the Dataflow worker is starting up.Fix
TestDataflowRunnerstaging directory undertempRootby UTC date (staging-<yyyy-MM-dd>) inTestDataflowRunner.fromOptionswhenever the default per-job staging path (<tempLocation>/staging) or unversioned shared staging path (<tempRoot>/staging) is used, while preserving custom explicit--stagingLocationvalues.TestDataflowRunnerpipelines executed on the same UTC day continue to share and reuse staged artifacts (via bothGcsStager.STAGED_FILES_CACHEandPackageUtil.alreadyStaged), while staged test objects are guaranteed to be< 24hold and never near the 7-day GCS lifecycle deletion threshold.PackageUtiluntouched so production pipelines reuse previously staged artifacts indefinitely as intended.Verification
./gradlew :runners:google-cloud-dataflow-java:spotlessCheck :runners:google-cloud-dataflow-java:test --tests org.apache.beam.runners.dataflow.TestDataflowRunnerTest :runners:google-cloud-dataflow-java:checkstyleMain :runners:google-cloud-dataflow-java:checkstyleTestPostCommit Java Examples Dataflow V2(beam_PostCommit_Java_Examples_Dataflow_V2.yml) on branchfix/dataflow-package-util-restage-expired.