[GitHub] [beam] damondouglas commented on a diff in pull request #28137: [WIP] Add a De-ID transform for Dicom IO

["damondouglas (via GitHub)" <gi...@apache.org>]

damondouglas commented on code in PR #28137:
URL: https://github.com/apache/beam/pull/28137#discussion_r1307644065


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java:
##########
@@ -51,8 +126,18 @@
  * readMetadataResult.getReadResponse() PCollection<String> failRead =
  * readMetadataResult.getFailedReads() }
  */
+
+ @SuppressWarnings({
+  "nullness" // TODO(https://github.com/apache/beam/issues/20497)

Review Comment:
   Can we remove this from the class? There are ways to achieve this by using methods in [org.apache.beam.sdk.util.Preconditions](https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/util/Preconditions.java)



##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java:
##########
@@ -196,4 +281,138 @@ public ReadStudyMetadata.Result expand(PCollection<String> input) {
                       TupleTagList.of(ReadStudyMetadata.ERROR_MESSAGE))));
     }
   }
+
+  /**
+   * Increments success and failure counters for an LRO. To be used after the LRO has completed.
+   * This function leverages the fact that the LRO metadata is always of the format: "counter": {
+   * "success": "1", "failure": "1" }
+   *
+   * @param operation LRO operation object.
+   * @param operationSuccessCounter the success counter for the operation.
+   * @param operationFailureCounter the failure counter for the operation.
+   * @param resourceSuccessCounter the success counter for individual resources in the operation.
+   * @param resourceFailureCounter the failure counter for individual resources in the operation.
+   */
+  private static void incrementLroCounters(
+      Operation operation,
+      Counter operationSuccessCounter,
+      Counter operationFailureCounter,
+      Counter resourceSuccessCounter,
+      Counter resourceFailureCounter) {

Review Comment:
   Can these be static in a single class?



##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/DicomIO.java:
##########
@@ -196,4 +281,138 @@ public ReadStudyMetadata.Result expand(PCollection<String> input) {
                       TupleTagList.of(ReadStudyMetadata.ERROR_MESSAGE))));
     }
   }
+
+  /**
+   * Increments success and failure counters for an LRO. To be used after the LRO has completed.
+   * This function leverages the fact that the LRO metadata is always of the format: "counter": {
+   * "success": "1", "failure": "1" }
+   *
+   * @param operation LRO operation object.
+   * @param operationSuccessCounter the success counter for the operation.
+   * @param operationFailureCounter the failure counter for the operation.
+   * @param resourceSuccessCounter the success counter for individual resources in the operation.
+   * @param resourceFailureCounter the failure counter for individual resources in the operation.
+   */
+  private static void incrementLroCounters(
+      Operation operation,
+      Counter operationSuccessCounter,
+      Counter operationFailureCounter,
+      Counter resourceSuccessCounter,
+      Counter resourceFailureCounter) {
+    // Update operation counters.
+    com.google.api.services.healthcare.v1.model.Status error = operation.getError();
+    if (error == null) {
+      operationSuccessCounter.inc();
+      LOG.debug(String.format("Operation %s finished successfully.", operation.getName()));
+    } else {
+      operationFailureCounter.inc();
+      LOG.error(
+          String.format(
+              "Operation %s failed with error code: %d and message: %s.",
+              operation.getName(), error.getCode(), error.getMessage()));
+    }
+
+    // Update resource counters.
+    Map<String, Object> opMetadata = operation.getMetadata();
+    if (opMetadata.containsKey(LRO_COUNTER_KEY)) {
+      try {
+        Map<String, String> counters = (Map<String, String>) opMetadata.get(LRO_COUNTER_KEY);
+        if (counters.containsKey(LRO_SUCCESS_KEY)) {
+          resourceSuccessCounter.inc(Long.parseLong(counters.get(LRO_SUCCESS_KEY)));
+        }
+        if (counters.containsKey(LRO_FAILURE_KEY)) {
+          Long numFailures = Long.parseLong(counters.get(LRO_FAILURE_KEY));
+          resourceFailureCounter.inc(numFailures);
+          if (numFailures > 0) {
+            LOG.error("Operation " + operation.getName() + " had " + numFailures + " failures.");
+          }
+        }
+      } catch (Exception e) {
+        LOG.error("failed to increment LRO counters, error message: " + e.getMessage());
+      }
+    }
+  }
+
+  /** Deidentify DICOM resources from a DICOM store to a destination DICOM store. */
+  public static class Deidentify extends PTransform<PBegin, PCollection<String>> {

Review Comment:
   Could this have the following signature instead:
   `PTransform<PBegin, PCollectionTuple>`
   or
   `PTransform<PBegin, Deidentify.Result>`
   where `Deidentify.Result` is a class similar to https://github.com/apache/beam/blob/master/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/FhirIO.java#L460



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org