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 2020/05/29 17:15:50 UTC

[GitHub] [beam] jaketf opened a new pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

jaketf opened a new pull request #11862:
URL: https://github.com/apache/beam/pull/11862


   Use timestamps / watermark estimate in `HL7v2IO.[Read, ListHL7v2Messages]` and provide interface to opt-in to `TimestampedValue<HL7v2Message>` to preserve the existing interface.
   
   R: @lukecwik 
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [x] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [x] 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.
    - [x] Update `CHANGES.md` with noteworthy changes.
    - [x] 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 | Apex | Dataflow | Flink | Gearpump | Samza | Spark
   --- | --- | --- | --- | --- | --- | --- | ---
   Go | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
   Java | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
   Python | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
   XLang | --- | --- | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [![Build Status](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
   
   Pre-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   --- |Java | Python | Go | Website
   --- | --- | --- | --- | ---
   Non-portable | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[![Build Status](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) 
   Portable | --- | [![Build Status](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon)](https://builds.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.
   


----------------------------------------------------------------
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] jaketf commented on pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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


   apologies I'm not working w/ healthcare clients these days and unable to prioritize this.
   I can take a look at this again after my current engagements end (~october)


----------------------------------------------------------------
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] stale[bot] commented on pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #11862:
URL: https://github.com/apache/beam/pull/11862#issuecomment-727715871


   This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
   


----------------------------------------------------------------
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] lukecwik commented on a change in pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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



##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -179,25 +189,51 @@ public static ListHL7v2Messages readAll(List<String> hl7v2Stores) {
     return new ListHL7v2Messages(StaticValueProvider.of(hl7v2Stores), StaticValueProvider.of(null));
   }
 
+  /** Read all HL7v2 Messages from multiple stores as sendTime {@link TimestampedValue}s. */
+  public static ListTimestampedHL7v2Messages readAllWithTimestamps(List<String> hl7v2Stores) {

Review comment:
       Instead of adding static methods for each variant, any reason to not make ListHL7v2Messages be a builder so someone could invoke `withMessageTimestamps`?
   
   This would have also make sense to make `withFilter` a builder like method on ListHL7v2Messages

##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -397,7 +492,8 @@ public void instantiateHealthcareClient() throws IOException {
         public void processElement(ProcessContext context) {
           String msgId = context.element();
           try {
-            context.output(HL7v2Message.fromModel(fetchMessage(this.client, msgId)));
+            HL7v2Message msg = HL7v2Message.fromModel(fetchMessage(this.client, msgId));
+            context.output(TimestampedValue.of(msg, Instant.parse(msg.getSendTime())));

Review comment:
       You should really be using `context.outputWithTimestamp` and not `TimestampedValue`.
   
   It would make sense to have an object that chooses which timestamp to use (input elements timestamp or messages send time) instead of having a different variant of the DoFn. In both cases you would use the monotonically increasing estimator.




----------------------------------------------------------------
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] aaltay commented on pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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


   retest this please


----------------------------------------------------------------
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] lukecwik commented on a change in pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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



##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -452,9 +454,16 @@ private Message fetchMessage(HealthcareApiClient client, String msgId)
    * with {@link ListHL7v2Messages#withInitialSplitDuration(Duration)}
    */
   public static class ListHL7v2Messages extends PTransform<PBegin, PCollection<HL7v2Message>> {
+    private static final TimestampMethod DEFAULT_TIMESTAMP_METHOD = TimestampMethod.SEND_TIME;
     private final ValueProvider<List<String>> hl7v2Stores;
-    private final ValueProvider<String> filter;
+    private ValueProvider<String> filter;
     private Duration initialSplitDuration;
+    private TimestampMethod timestampMethod;
+
+    enum TimestampMethod {

Review comment:
       comment?

##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -466,21 +475,63 @@ private Message fetchMessage(HealthcareApiClient client, String msgId)
       this.hl7v2Stores = hl7v2Stores;
       this.filter = filter;
       this.initialSplitDuration = null;
+      this.timestampMethod = DEFAULT_TIMESTAMP_METHOD;
     }
 
-    public ListHL7v2Messages withInitialSplitDuration(Duration initialSplitDuration) {
+    /**
+     * Controls the initial splitting for parallelization of HL7v2 Messages List requests based on
+     * disjoint sendTime filters of the specified duration.
+     *
+     * @param initialSplitDuration the initial split duration
+     * @return the list hl 7 v 2 messages
+     */
+    ListHL7v2Messages withInitialSplitDuration(Duration initialSplitDuration) {
       this.initialSplitDuration = initialSplitDuration;

Review comment:
       Please return a new version of this transform instead of mutating the existing version.
   
   It is not uncommon for users to take one transform apply it to the graph once and then call withX on it and apply it elsewhere. We wouldn't want the second mutation to ever impact the first application of the transform and it is easy to guard against it using the @AutoValue builders.

##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -596,9 +664,14 @@ public void listMessages(
           lastClaimedMilliSecond = cursor.getMillis();
         }
 
-        outputReceiver.output(msg);
+        switch (timestampMethod) {
+          case SEND_TIME:
+            outputReceiver.outputWithTimestamp(msg, Instant.parse(msg.getSendTime()));
+            break;
+          case INPUT_ELEMENT:
+            outputReceiver.outputWithTimestamp(msg, context.timestamp());

Review comment:
       You should use output(msg) and not outputWithTimestamp since output() by default does this. This also allows you to not have to take a ProcessContext parameter.

##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -466,21 +475,63 @@ private Message fetchMessage(HealthcareApiClient client, String msgId)
       this.hl7v2Stores = hl7v2Stores;
       this.filter = filter;
       this.initialSplitDuration = null;
+      this.timestampMethod = DEFAULT_TIMESTAMP_METHOD;
     }
 
-    public ListHL7v2Messages withInitialSplitDuration(Duration initialSplitDuration) {
+    /**
+     * Controls the initial splitting for parallelization of HL7v2 Messages List requests based on
+     * disjoint sendTime filters of the specified duration.
+     *
+     * @param initialSplitDuration the initial split duration
+     * @return the list hl 7 v 2 messages

Review comment:
       ?

##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -466,21 +475,63 @@ private Message fetchMessage(HealthcareApiClient client, String msgId)
       this.hl7v2Stores = hl7v2Stores;
       this.filter = filter;
       this.initialSplitDuration = null;
+      this.timestampMethod = DEFAULT_TIMESTAMP_METHOD;
     }
 
-    public ListHL7v2Messages withInitialSplitDuration(Duration initialSplitDuration) {
+    /**
+     * Controls the initial splitting for parallelization of HL7v2 Messages List requests based on
+     * disjoint sendTime filters of the specified duration.
+     *
+     * @param initialSplitDuration the initial split duration
+     * @return the list hl 7 v 2 messages
+     */
+    ListHL7v2Messages withInitialSplitDuration(Duration initialSplitDuration) {
       this.initialSplitDuration = initialSplitDuration;
       return this;
     }
 
+    /**
+     * Controls if the output elements will be assigned a timestamp beased on the sendTime property
+     * or the input element's timestamp.
+     *
+     * @param timestampMethod the timestamp method
+     * @return the list hl 7 v 2 messages
+     */
+    ListHL7v2Messages withTimestampMethod(TimestampMethod timestampMethod) {

Review comment:
       How about `withOutputTimestampMethod`, also check that the argument is non-null?
   
   nit: I'm torn between using an enum or just having the two variants between methods. Your call. Having two variants prevents the user from passing in null here.

##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -509,13 +561,17 @@ public ListHL7v2Messages withInitialSplitDuration(Duration initialSplitDuration)
      * @param filter the filter
      */
     ListHL7v2MessagesFn(String filter) {
-      this(StaticValueProvider.of(filter), null);
+      this(StaticValueProvider.of(filter), null, null);

Review comment:
       We pass in null to the constructor and don't handle that case in the switch statement. How about passing in the default?

##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -466,21 +475,63 @@ private Message fetchMessage(HealthcareApiClient client, String msgId)
       this.hl7v2Stores = hl7v2Stores;
       this.filter = filter;
       this.initialSplitDuration = null;
+      this.timestampMethod = DEFAULT_TIMESTAMP_METHOD;
     }
 
-    public ListHL7v2Messages withInitialSplitDuration(Duration initialSplitDuration) {
+    /**
+     * Controls the initial splitting for parallelization of HL7v2 Messages List requests based on
+     * disjoint sendTime filters of the specified duration.
+     *
+     * @param initialSplitDuration the initial split duration
+     * @return the list hl 7 v 2 messages
+     */
+    ListHL7v2Messages withInitialSplitDuration(Duration initialSplitDuration) {
       this.initialSplitDuration = initialSplitDuration;
       return this;
     }
 
+    /**
+     * Controls if the output elements will be assigned a timestamp beased on the sendTime property
+     * or the input element's timestamp.
+     *
+     * @param timestampMethod the timestamp method
+     * @return the list hl 7 v 2 messages

Review comment:
       ?

##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -90,9 +91,9 @@
  * is mainly to catch scenarios where the upstream {@link PCollection} contains IDs that are not
  * valid or are not reachable due to permissions issues.
  *
- * <p>Message Listing Message Listing with {@link HL7v2IO.ListHL7v2Messages} supports batch use
- * cases where you want to process all the messages in an HL7v2 store or those matching a
- * filter @see <a
+ * <p>Message Listing Message Listing with {@link ListHL7v2Messages} and {@link ListHL7v2Messages}

Review comment:
       ?
   ```suggestion
    * <p>Message Listing with {@link ListHL7v2Messages} and {@link ListHL7v2Messages}
   ```

##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -466,21 +475,63 @@ private Message fetchMessage(HealthcareApiClient client, String msgId)
       this.hl7v2Stores = hl7v2Stores;
       this.filter = filter;
       this.initialSplitDuration = null;
+      this.timestampMethod = DEFAULT_TIMESTAMP_METHOD;
     }
 
-    public ListHL7v2Messages withInitialSplitDuration(Duration initialSplitDuration) {
+    /**
+     * Controls the initial splitting for parallelization of HL7v2 Messages List requests based on
+     * disjoint sendTime filters of the specified duration.
+     *
+     * @param initialSplitDuration the initial split duration
+     * @return the list hl 7 v 2 messages
+     */
+    ListHL7v2Messages withInitialSplitDuration(Duration initialSplitDuration) {
       this.initialSplitDuration = initialSplitDuration;
       return this;
     }
 
+    /**
+     * Controls if the output elements will be assigned a timestamp beased on the sendTime property
+     * or the input element's timestamp.

Review comment:
       ```suggestion
        * Controls if the output elements will be assigned a timestamp beased on the {@code sendTime} property
        * or the input element's timestamp.
   ```




----------------------------------------------------------------
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] jaketf commented on a change in pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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



##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -179,25 +189,51 @@ public static ListHL7v2Messages readAll(List<String> hl7v2Stores) {
     return new ListHL7v2Messages(StaticValueProvider.of(hl7v2Stores), StaticValueProvider.of(null));
   }
 
+  /** Read all HL7v2 Messages from multiple stores as sendTime {@link TimestampedValue}s. */
+  public static ListTimestampedHL7v2Messages readAllWithTimestamps(List<String> hl7v2Stores) {

Review comment:
       this is a good point.
   
   Unfortunately, I think having adding replacing the existing static methods with `withFilter` builder would be interface breaking. I've added this method to `ListHL7v2Messages` as it might be more natural for some to use.




----------------------------------------------------------------
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] stale[bot] commented on pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #11862:
URL: https://github.com/apache/beam/pull/11862#issuecomment-723500382


   This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.
   


----------------------------------------------------------------
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] lukecwik commented on a change in pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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



##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/healthcare/HL7v2IO.java
##########
@@ -179,25 +189,51 @@ public static ListHL7v2Messages readAll(List<String> hl7v2Stores) {
     return new ListHL7v2Messages(StaticValueProvider.of(hl7v2Stores), StaticValueProvider.of(null));
   }
 
+  /** Read all HL7v2 Messages from multiple stores as sendTime {@link TimestampedValue}s. */
+  public static ListTimestampedHL7v2Messages readAllWithTimestamps(List<String> hl7v2Stores) {

Review comment:
       I think you can keep the static methods that exist and transition them to use the builder underneath so you don't have to break the interface. This is done in other IOs for common scenarios.




----------------------------------------------------------------
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] iemejia removed a comment on pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

Posted by GitBox <gi...@apache.org>.
iemejia removed a comment on pull request #11862:
URL: https://github.com/apache/beam/pull/11862#issuecomment-636342168


   retest this please


----------------------------------------------------------------
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] aaltay commented on pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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


   @jaketf - Is this PR still active?


----------------------------------------------------------------
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] aaltay commented on pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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


   retest this please


----------------------------------------------------------------
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] iemejia commented on pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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


   retest this please


----------------------------------------------------------------
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] lukecwik commented on pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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


   Ping?


----------------------------------------------------------------
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] aaltay commented on pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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


   retest this please


----------------------------------------------------------------
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] stale[bot] closed pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

Posted by GitBox <gi...@apache.org>.
stale[bot] closed pull request #11862:
URL: https://github.com/apache/beam/pull/11862


   


----------------------------------------------------------------
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] lukecwik commented on pull request #11862: [BEAM-10141] HL7v2 io timestamps / watermark estimate

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


   Sorry for the long delay.


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