[runners-spark] Spark 4 follow-up: Announce in CHANGES.md and address remaining review comments from #38255#38489
Conversation
…from apache#38255 Addresses two trailing review comments left on PR apache#38255 at the approved/merged head SHA that were never folded in before merge: - Restore the `feature Y added to Java SDK` placeholder line in CHANGES.md Highlights (lost to a rebase). Comment: apache#38255 (comment) - Delete two orphaned `.github/trigger_files/*.json`: - `beam_PreCommit_Java_Spark4_Versions.json`: its workflow YAML was deleted in apache#38255. - `beam_PostCommit_Java_ValidatesRunner_Spark4StructuredStreaming.json`: flagged as "not effective yet"; since then, apache#38478 renamed the associated workflow YAML to `beam_PostCommit_Java_ValidatesRunner_Spark4.yml`, so this trigger JSON is doubly orphaned. Comment: apache#38255 (comment) The renamed workflow YAML stays in place; per the trigger-files convention these JSONs are only added when an author wants to manually trigger a specific PostCommit.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request serves as a follow-up to the Spark 4 runner implementation, addressing minor review comments that were missed during the initial merge. The changes focus on maintaining documentation accuracy and cleaning up obsolete configuration files. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request involves the deletion of CI trigger files and an update to the release notes in CHANGES.md. Feedback indicates that deleting the trigger files without renaming them to match the new workflow name will break manual CI triggers. Additionally, it is suggested to explicitly list the Spark 4 runner in the highlights section of the release notes rather than just using a placeholder.
| @@ -1,3 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
The PR description mentions that the associated workflow was renamed to beam_PostCommit_Java_ValidatesRunner_Spark4.yml. In the Beam repository, these trigger files are used by the CI system to allow manual triggering of jobs via GitHub comments (e.g., for ValidatesRunner tests). Deleting this file without providing a replacement with the new name (beam_PostCommit_Java_ValidatesRunner_Spark4.json) will break the ability for contributors to manually trigger these tests. It is recommended to rename the file to match the new workflow name instead of just deleting it.
There was a problem hiding this comment.
Thanks @gemini-code-assist, but my read of @Abacn's guidance on #38453 is that we don't add stub trigger JSONs for new workflow YAMLs. Quoting his reply on #38453 (#38453 (comment)):
I've seen a few occurrences AI-assisted PRs keep adding trigger file json for new workflow addition, which is not necessary. Creation or modification of trigger file is only needed when the PR author want to trigger a specific PostCommit manually.
So the deletion here removes a doubly-orphaned file (workflow YAML deleted in #38255, then renamed in #38478), and we deliberately do not add a beam_PostCommit_Java_ValidatesRunner_Spark4.json replacement. Manual triggering still works via the standard Run Java_ValidatesRunner_Spark4 PostCommit comment phrase that the workflow YAML defines; the trigger JSON is only one of several activation paths.
There was a problem hiding this comment.
Thank you for the clarification regarding the trigger file policy. Understanding that these files are not required for new workflows and that manual triggering is handled via the workflow YAML directly clarifies the situation.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Assigning reviewers: R: @Abacn added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Co-authored-by: Yi Hu <huuyyi@gmail.com>
Small follow-up to PR #38255 (
[runners-spark] Add Spark 4 runner) addressing the two review comments left at the approved/merged head SHA that weren't folded in before merge.Changes
Restore
CHANGES.mdfeature Y added to Java SDKplaceholder line — lost to a rebase; reinstated verbatim. Addressesr3204237226.Delete two orphaned
.github/trigger_files/*.json- addressesr3204241059:beam_PreCommit_Java_Spark4_Versions.json- its workflow YAML was deleted in [runners-spark] Add Spark 4 runner #38255.beam_PostCommit_Java_ValidatesRunner_Spark4StructuredStreaming.json- flagged as "not effective yet"; since the comment was written, Setup Validates runner tests for Spark4 runner #38478 renamed the associated workflow YAML tobeam_PostCommit_Java_ValidatesRunner_Spark4.yml, so this trigger JSON is doubly orphaned.The renamed workflow YAML stays in place - per the trigger-files convention, these JSONs are only added when an author wants to manually trigger a specific PostCommit.
Verification
:validateChangespassesR: @Abacn