You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/03/15 15:53:50 UTC

[GitHub] [beam] macksclark opened a new pull request #14233: Adding LRO counters to FhirIO

macksclark opened a new pull request #14233:
URL: https://github.com/apache/beam/pull/14233


   Adding counters for the success/failure counters that are output in the operation metadata after an LRO completes. We increment the counters after the operation is complete since we already wait on them. 
   
   Tested that this change works properly for Import but I don't have tests for the other LROs.
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   Post-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
   --- | --- | --- | --- | --- | --- | ---
   Go | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
   Java | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon)](https://ci-beam
 .apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [![Build Status](https://ci-beam.a
 pache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
   Python | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[![Build Status](https://ci-beam
 .apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
   XLang | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
   
   Pre-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   --- |Java | Python | Go | Website | Whitespace | Typescript
   --- | --- | --- | --- | --- | --- | ---
   Non-portable | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/be
 am_PreCommit_Go_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
   Portable | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
   
   See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
   
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


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



[GitHub] [beam] pabloem commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-800602523


   thanks ! seems like you need to fix the formatting.
   
   Regarding the counter checks: I don't think they're necessary. Feel free to add them if you'd like.


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



[GitHub] [beam] macksclark commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-802286536


   These two tests are failing pretty reliably (they've failed every run for the past 3)
   
   org.apache.beam.sdk.io.gcp.pubsub.PubsubReadIT.testReadPubsubMessageId
   org.apache.beam.sdk.io.gcp.pubsub.PubsubReadIT.testReadPublicData
   
   Is that expected?


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



[GitHub] [beam] macksclark commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-802339040


   Thanks Pablo!


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



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

Posted by GitBox <gi...@apache.org>.
macksclark commented on a change in pull request #14233:
URL: https://github.com/apache/beam/pull/14233#discussion_r595526804



##########
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:
       We are technically guaranteed that this will be a map since the GetOperation method is in GA with a 2 year deprecation period (if it's ever decided to deprecate or change it) - but I put it all in a try catch block anyways.




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



[GitHub] [beam] macksclark commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-801421069


   Run Java PostCommit


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



[GitHub] [beam] macksclark commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-801981620


   Run Java PostCommit


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



[GitHub] [beam] macksclark commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-801177858


   Can we hold off on merging this? I got some internal feedback that maybe we should change the counter names like:
   import_success_count -> import_resource_success_count 
   
   Or something similar, because import_success_count could easily indicate the entire import succeeded, which is not what we are trying to track with these counters. Please let me know if you have an opinion on this naming convention. Thanks!


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



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

Posted by 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



[GitHub] [beam] macksclark commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-801216271


   Run Java PostCommit


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



[GitHub] [beam] macksclark commented on pull request #14233: Adding LRO counters to FhirIO

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-799531814


   Run Spotless PreCommit


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



[GitHub] [beam] macksclark commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-800606413


   Thanks! I won't worry about the counter checks for now.


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



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

Posted by GitBox <gi...@apache.org>.
macksclark commented on a change in pull request #14233:
URL: https://github.com/apache/beam/pull/14233#discussion_r595528069



##########
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:
       I agree, although we still need to check if the keys exist because, for example if there are no failures in the LRO, the counters will look like:
   {"counters":{"success":"1"}} - with no failure field at all.
   
   Catching all exceptions as discussed because we don't want to block anything on this function.




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



[GitHub] [beam] macksclark commented on pull request #14233: Adding LRO counters to FhirIO

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-799547640


   R: @pabloem 


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



[GitHub] [beam] macksclark commented on pull request #14233: Adding LRO counters to FhirIO

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-799548320


   Hi Pablo, can you please take a look at this PR? I'm not sure if the tests are failing because of my change or if they were already failing. Thanks!


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



[GitHub] [beam] pabloem merged pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
pabloem merged pull request #14233:
URL: https://github.com/apache/beam/pull/14233


   


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



[GitHub] [beam] pabloem commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-800649831


   Run Java PostCommit


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



[GitHub] [beam] pabloem commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-802334728


   Thanks for retrying the tests. I think this is fine. We'll get someone to address these other tests which you clearly didn't affect in this change.


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



[GitHub] [beam] macksclark commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-801215925


   Updated. Pablo, if you approve of the new names it's good to merge. Thanks!


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



[GitHub] [beam] macksclark commented on pull request #14233: Adding LRO counters to FhirIO

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-800593683


   Regarding the tests you mentioned running - they are passing reliably for me (I have yet to see any flakes) - did you want me to add asserts on the counters I just added, or would that be redundantly verifying that the LROs worked properly?


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



[GitHub] [beam] macksclark commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
macksclark commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-801116270


   Run Java PostCommit


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



[GitHub] [beam] pabloem commented on pull request #14233: [BEAM-11733] Adding LRO counters to FhirIO and re-enable FhirIO tests

Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #14233:
URL: https://github.com/apache/beam/pull/14233#issuecomment-800634173


   Run Java_Examples_Dataflow PreCommit


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