-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add PatchResources to FhirIO. #14691
Conversation
References: https://cloud.google.com/healthcare/docs/reference/rest/v1beta1/projects.locations.datasets.fhirStores.fhir/patch https://cloud.google.com/healthcare/docs/how-tos/fhir-resources#patching_a_fhir_resource https://cloud.google.com/healthcare/docs/how-tos/fhir-resources#conditionally_patching_a_fhir_resource The Google Cloud FHIR service doesn't support neither Patch nor Conditional Patch within bundles, so it requires it's own connector.
R: @pabloem |
there's spotless errors: https://ci-beam.apache.org/job/beam_PreCommit_Spotless_Commit/15767/console You can run the There's also a compile error: https://scans.gradle.com/s/y7kzzhs6dsx5g/console-log?task=:sdks:java:io:google-cloud-platform:compileJava |
also please use Beam's vendored guava : ) - https://scans.gradle.com/s/z7ynu5c4rnrb4/console-log?task=:sdks:java:io:google-cloud-platform:checkstyleTest |
I think everything should be fixed now :) |
Run Java PreCommit |
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.
Added a couple comments.
...ava/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/FhirIO.java
Outdated
Show resolved
Hide resolved
...e-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/FhirPatchParameter.java
Outdated
Show resolved
Hide resolved
...-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HealthcareApiClient.java
Show resolved
Hide resolved
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.
LGTM. Thanks! Just the one comment.
@@ -1417,8 +1432,107 @@ public void executeBundles(ProcessContext context) { | |||
} | |||
} | |||
|
|||
/** The type Patch resources. */ | |||
public static class PatchResources extends PTransform<PCollection<Input>, Write.Result> { | |||
|
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.
it might be good to add a non-public constructor here to ensure users won't create this transform directly, and they will use the FhirIO.patchResources directly. Thoughts?
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.
I agree it makes sense, added a private constructor.
Run Java PostCommit |
LGTM. I'll merge once postcommits are passing. |
It looks like somehow this broke beam_PostCommit_Java_DataflowV2 (BEAM-12310), could someone take a look? |
hm sorry about that. I missed running the Dataflow postcommits. @msbukal do you have time to take a look? |
In the failure linked, it looks like it's failing to read resources/ files on line: I'm not sure how the failure could be introduced by this change, as I didn't modify this file at all. I also can't reproduce the error so not sure how to look further, is this a flake or a consistent failure? |
hmm here' sthe history of test suite runs: https://ci-beam.apache.org/job/beam_PostCommit_Java_DataflowV2/ |
Can we try rolling back? It is a consistent failure. Note that the failures are both in |
For a repro, you will need to run on Dataflow v2, it seems fine on Dataflow v1. The simplest way is probably to just execute the same task used in the Jenkins job:
|
I get that it's coming from FhirIOPatchIT, but I can't find any differences between how FhirIOPatchIT and FhirIO[X]IT call |
Yeah I agree it's surprising, but since it's clear this PR broke a PostCommit we should roll it back first, then diagnose why. Please see https://beam.apache.org/contribute/postcommits-policies-details/#rollback_first |
From https://ci-beam.apache.org/job/beam_PostCommit_Java_DataflowV2/, we can see that the failure is consistent. Do you want to propose a rollback or you want me to help on that? |
Ah sorry, I wasn't aware that the rollback is something I am supposed to do. How do you make a rollback? I might not have the time to do this. |
…irIO." This reverts commit e37bede
Sent out revert PR: #14816 |
…#14691 from Add PatchResources to FhirIO." [BEAM-12310] Revert "Merge pull request #14691 from Add PatchResources to FhirIO."
References:
https://cloud.google.com/healthcare/docs/reference/rest/v1beta1/projects.locations.datasets.fhirStores.fhir/patch
https://cloud.google.com/healthcare/docs/how-tos/fhir-resources#patching_a_fhir_resource
https://cloud.google.com/healthcare/docs/how-tos/fhir-resources#conditionally_patching_a_fhir_resource
The Google Cloud FHIR service doesn't support neither Patch nor Conditional Patch within bundles, so it requires its own connector.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
ValidatesRunner
compliance status (on master branch)Examples testing status on various runners
Post-Commit SDK/Transform Integration Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.