-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Add pipeline option to force GBEK (Java) #36346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Assigning reviewers: R: @Abacn for label java. 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). |
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is a great feature addition for enhancing security. The implementation looks solid, with good separation of concerns and extensibility. I've made a few suggestions to improve documentation and code clarity. One larger point is about the new integration tests: there's some duplicated test setup logic for GCP Secret Manager in GroupByKeyTest and GroupByKeyIT. It would be beneficial to refactor this into a shared utility or a JUnit @Rule. Additionally, the new tests in GroupByKeyTest that require a live GCP environment would be better placed in GroupByKeyIT to maintain a clear separation between unit and integration tests. Overall, excellent work!
sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/GroupByEncryptedKey.java
Outdated
Show resolved
Hide resolved
|
Test failures are all unrelated |
Abacn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, had a pass for main codes
...ataflow-java/src/main/java/org/apache/beam/runners/dataflow/internal/DataflowGroupByKey.java
Show resolved
Hide resolved
...ataflow-java/src/main/java/org/apache/beam/runners/dataflow/internal/DataflowGroupByKey.java
Outdated
Show resolved
Hide resolved
...ataflow-java/src/main/java/org/apache/beam/runners/dataflow/internal/DataflowGroupByKey.java
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptions.java
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/GroupByKey.java
Show resolved
Hide resolved
...ataflow-java/src/main/java/org/apache/beam/runners/dataflow/internal/DataflowGroupByKey.java
Show resolved
Hide resolved
sdks/java/core/src/main/java/org/apache/beam/sdk/util/construction/GroupByKeyTranslation.java
Outdated
Show resolved
Hide resolved
|
Not sure what happened underlying, but this appears breaking https://github.com/apache/beam/actions/workflows/beam_PostCommit_Java_PVR_Spark3_Streaming.yml (#34207) last good run: https://github.com/apache/beam/actions/runs/18327770129 first failing run: https://github.com/apache/beam/actions/runs/18333678979 only differs by one commit (c8df4da) that is this one |
Ack - kicked off a debugging run in a CI env here - #36454 - with this/subsequent gbek commits reverted. Assuming that succeeds I'll go from there. Its very unclear what in this PR would have caused this failure, but I agree it is suspicious EDIT: Fixed by #36479 (test environment was getting polluted somehow) |
Adds a pipeline option to auto-replace GBEK with encrypted GBEK in Java. To support this, makes the following changes:
I'd recommend reviewing things in that order, I think it will help make sense of the PR.
Java version of #36321
Part of #36214