[
https://issues.apache.org/jira/browse/BEAM-14504?focusedWorklogId=774772&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-774772
]
ASF GitHub Bot logged work on BEAM-14504:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 25/May/22 18:53
Start Date: 25/May/22 18:53
Worklog Time Spent: 10m
Work Description: msbukal commented on code in PR #17741:
URL: https://github.com/apache/beam/pull/17741#discussion_r881954049
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/FhirBundleWithMetadata.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.gcp.healthcare;
+
+import com.google.auto.value.AutoValue;
+import java.util.Objects;
+import javax.annotation.Nullable;
+import org.apache.beam.sdk.coders.DefaultCoder;
+
+/**
+ * FhirBundleWithMetadata represents a FHIR bundle, with it's metadata (eg.
source ID like HL7
Review Comment:
I think we should change the name from "FhirBundleWithMetadata" to something
more generic, since now ExecuteBundle MUST accepts this as input. How about
"FhirBundleParameter", similar to the input to search (FhirSearchParameter).
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/FhirBundleWithMetadata.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.gcp.healthcare;
+
+import com.google.auto.value.AutoValue;
+import java.util.Objects;
+import javax.annotation.Nullable;
+import org.apache.beam.sdk.coders.DefaultCoder;
+
+/**
+ * FhirBundleWithMetadata represents a FHIR bundle, with it's metadata (eg.
source ID like HL7
+ * message path) in JSON format to be executed on any FHIR store. *
+ */
+@DefaultCoder(FhirBundleWithMetadataCoder.class)
+@AutoValue
+public abstract class FhirBundleWithMetadata {
+
+ static Builder builder() {
+ return new AutoValue_FhirBundleWithMetadata.Builder();
+ }
+
+ /**
+ * String representing the source of the Bundle to be written. Used to pass
source data through
+ * the ExecuteBundles PTransform.
+ */
+ public abstract String getMetadata();
Review Comment:
Remove occurrences of "source" -> "metadata", since it doesn't have to be
the source.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/FhirBundleWithMetadata.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.gcp.healthcare;
+
+import com.google.auto.value.AutoValue;
+import java.util.Objects;
+import javax.annotation.Nullable;
+import org.apache.beam.sdk.coders.DefaultCoder;
+
+/**
+ * FhirBundleWithMetadata represents a FHIR bundle, with it's metadata (eg.
source ID like HL7
+ * message path) in JSON format to be executed on any FHIR store. *
+ */
+@DefaultCoder(FhirBundleWithMetadataCoder.class)
+@AutoValue
+public abstract class FhirBundleWithMetadata {
+
+ static Builder builder() {
+ return new AutoValue_FhirBundleWithMetadata.Builder();
+ }
+
+ /**
+ * String representing the source of the Bundle to be written. Used to pass
source data through
+ * the ExecuteBundles PTransform.
+ */
+ public abstract String getMetadata();
+
+ /** FHIR R4 bundle resource object as a string. */
+ public abstract String getBundle();
+
+ /** HTTP response from the FHIR store after attempting to write the Bundle
method. */
+ public abstract String getResponse();
Review Comment:
It seems confusing to include the "response" field in the input to an
ExecuteBundle request. Especially since the only 2 constructors are "bundle
only" or "bundle, metadata, and response". How would you create a bundle with
metadata as input?
I think the solution here should be to make the response of the
ExecuteBundle DoFn it's own "FhirBundleResponse" class, which can contain a
FhirBundleWithMetadata/FhirBundleParameter inside it (or just the two fields,
this might be easier regarding serialization, either is fine) as the "source".
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/FhirBundleWithMetadata.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.gcp.healthcare;
+
+import com.google.auto.value.AutoValue;
+import java.util.Objects;
+import javax.annotation.Nullable;
+import org.apache.beam.sdk.coders.DefaultCoder;
+
+/**
+ * FhirBundleWithMetadata represents a FHIR bundle, with it's metadata (eg.
source ID like HL7
+ * message path) in JSON format to be executed on any FHIR store. *
Review Comment:
Comment nits:
* it's -> its
* any FHIR store -> a FHIR store (specifying any can be confusing, it's the
specific FHIR store that a user passes in)
* change the example from "HL7v2 message path" to something more generic,
eg. the Pubsub Notification ID (since most executes come from reads), don't
need to bring up another modality.
* mention the metadata seperately: emphasize the main purpose first:
`FhirBundleWithMetadata represents a FHIR bundle in JSON format to be
executed on a FHIR store`
then mention that the passed in metadata will be returned in the response,
and can be used for tracking metadata such as, for example, the source.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/FhirIO.java:
##########
@@ -1496,16 +1517,22 @@ private void parseResponse(ProcessContext context,
String inputBody, HttpBody re
} else {
fail++;
context.output(
- Write.FAILED_BODY,
+ FAILED_BUNDLES,
HealthcareIOError.of(
- inputBody, HealthcareHttpException.of(statusCode,
entry.toString())));
+ context.element(),
HealthcareHttpException.of(statusCode, entry.toString())));
}
}
EXECUTE_BUNDLE_RESOURCE_SUCCESS.inc(success);
EXECUTE_BUNDLE_RESOURCE_ERRORS.inc(fail);
} else if (bundleType.equals(BUNDLE_RESPONSE_TYPE_TRANSACTION)) {
EXECUTE_BUNDLE_RESOURCE_SUCCESS.inc(entries.size());
- context.output(Write.SUCCESSFUL_BODY, bundle.toString());
+ context.output(
Review Comment:
In the BATCH case above (line 1516) we output to Write.SUCCESSFUL_BODY,
which the PTransform no longer returns, since the output tags are only:
`.withOutputTags(SUCCESSFUL_BUNDLES, TupleTagList.of(FAILED_BUNDLES)));`
This means that nothing will be returned in the batch case. If you run the
integration tests (can comment `Run Java PostCommit`) it'll fail.
Please fix.
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/GetStringHealthcareIOErrorFn.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.io.gcp.healthcare;
+
+import org.apache.beam.sdk.transforms.DoFn;
+
+/**
+ * ParDo function that transforms a HealthcareIOError for a
FhirBundleWithMetadata to an error with
+ * the body (string) as the data resource, for backwards compatibility.
+ */
+public class GetStringHealthcareIOErrorFn
Review Comment:
I agree with Mackenzie's comment: This shouldn't be it's own class/DoFN: you
can just make a helper method somewhere local in FhirIO, like:
```
private HealthcareIOError<String>
getStringHealthcareIOError(HealthcareIOError<FhirBundleWithMetadata> in) {
return new HealthcareIOError<>(
input.getDataResource().getBundle(),
input.getErrorMessage(),
input.getStackTrace(),
input.getObservedTime(),
input.getStatusCode()));
}
```
and apply MapElements to the Results.getFailedBodies() method in-line (like
some of the other results methods)
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/FhirIO.java:
##########
@@ -1524,6 +1551,85 @@ private int parseBundleStatus(String status) {
}
}
+ /**
+ * ExecuteBundlesResult contains both successfully executed bundles and
information help debugging
+ * failed executions (eg metadata & error msgs).
+ */
+ public static class ExecuteBundlesResult extends Write.AbstractResult {
+ private final Pipeline pipeline;
+ private final PCollection<FhirBundleWithMetadata> successfulBundles;
+ private final PCollection<HealthcareIOError<FhirBundleWithMetadata>>
failedBundles;
+
+ private ExecuteBundlesResult(
+ Pipeline pipeline,
+ PCollection<FhirBundleWithMetadata> successfulBundles,
+ PCollection<HealthcareIOError<FhirBundleWithMetadata>> failedBundles) {
+ this.pipeline = pipeline;
+ this.successfulBundles = successfulBundles;
+ this.failedBundles = failedBundles;
+ }
+
+ /**
+ * Entry point for the ExecuteBundlesResult, storing the successful and
failed bundles and their
+ * metadata.
+ */
+ public static ExecuteBundlesResult in(
+ Pipeline pipeline,
+ PCollection<FhirBundleWithMetadata> successfulBundles,
+ PCollection<HealthcareIOError<FhirBundleWithMetadata>> failedBundles) {
+ return new ExecuteBundlesResult(pipeline, successfulBundles,
failedBundles);
+ }
+
+ /** Gets successful FhirBundleWithMetadata from execute bundles operation.
*/
+ public PCollection<FhirBundleWithMetadata>
getSuccessfulBundlesWithMetadata() {
+ return this.successfulBundles;
+ }
+
+ /**
+ * Gets failed FhirBundleWithMetadata with metadata wrapped inside
HealthcareIOError. The bundle
+ * field could be null.
+ */
+ public PCollection<HealthcareIOError<FhirBundleWithMetadata>>
getFailedBundles() {
+ return this.failedBundles;
+ }
+
+ @Override
Review Comment:
nit: order the methods to group success/fails, eg:
getSuccessfulBodies()
getSuccessfulBundlesWithMetadata()
getFailedBodies()
getFailedBundles()
makes it easier to see the difference between methods.
Issue Time Tracking
-------------------
Worklog Id: (was: 774772)
Time Spent: 1.5h (was: 1h 20m)
> Add support for including addittional parameters to executebundle method in
> fhirio.
> -----------------------------------------------------------------------------------
>
> Key: BEAM-14504
> URL: https://issues.apache.org/jira/browse/BEAM-14504
> Project: Beam
> Issue Type: Improvement
> Components: io-java-healthcare
> Reporter: Fathima Mohammed
> Assignee: Fathima Mohammed
> Priority: P2
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> Add FhirBundleWithMetadata in executebundles method so that we can pass
> additional information like message id.
> FhirBundleWithMetadata represents a FHIR bundle, with it's metadata (eg. Hl7
> messageId) to be executed on the intermediate FHIR store.
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
