[GitHub] [beam] pabloem commented on a change in pull request #14233: Adding LRO counters to FhirIO

[GitBox <gi...@apache.org>]

pabloem commented on a change in pull request #14233:
URL: https://github.com/apache/beam/pull/14233#discussion_r594632249



##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/FhirIO.java
##########
@@ -378,6 +381,29 @@ public static Deidentify deidentify(
     return new Deidentify(sourceFhirStore, destinationFhirStore, deidConfig);
   }
 
+  /**
+   * 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 successCounter the success counter for this operation.
+   * @param failureCounter the failure counter for this operation.
+   */
+  private static void incrementLroCounters(
+      Operation operation, Counter successCounter, Counter failureCounter) {
+    Map<String, Object> opMetadata = operation.getMetadata();
+    if (opMetadata.containsKey(LRO_COUNTER_KEY)) {
+      Map<String, String> counters = (Map<String, String>) opMetadata.get(LRO_COUNTER_KEY);

Review comment:
       are we guaranteed that this will always be a map? should we handle any potential cast exceptions?

##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/FhirIO.java
##########
@@ -378,6 +381,29 @@ public static Deidentify deidentify(
     return new Deidentify(sourceFhirStore, destinationFhirStore, deidConfig);
   }
 
+  /**
+   * 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 successCounter the success counter for this operation.
+   * @param failureCounter the failure counter for this operation.
+   */
+  private static void incrementLroCounters(
+      Operation operation, Counter successCounter, Counter failureCounter) {
+    Map<String, Object> opMetadata = operation.getMetadata();
+    if (opMetadata.containsKey(LRO_COUNTER_KEY)) {
+      Map<String, String> counters = (Map<String, String>) opMetadata.get(LRO_COUNTER_KEY);
+      if (counters.containsKey(LRO_SUCCESS_KEY)) {
+        successCounter.inc(Long.parseLong(counters.get(LRO_SUCCESS_KEY)));

Review comment:
       maybe we can handle cast / parsing exceptions in a single try/catch block?
   
   I understand that this is highly unlikely, but since we don't have a way to guarantee this at compilation time, we could be stuck with pipelines failing indefinitely if the API were to change in any way in the future and we were to forget to update this.




----------------------------------------------------------------
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.

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