Add Ordered Processing PTransform to Java SDK#30735
Conversation
damccorm
left a comment
damccorm
left a comment
There was a problem hiding this comment.
Didn't quite make it through the whole PR yet, but I think I hit the biggest pieces. Overall, this looks good, thanks! Comments are mostly minor
...a/extensions/ordered/src/main/java/org/apache/beam/sdk/extensions/ordered/EventExaminer.java
Outdated
Show resolved
Hide resolved
...va/extensions/ordered/src/main/java/org/apache/beam/sdk/extensions/ordered/MutableState.java
Outdated
Show resolved
Hide resolved
...ions/ordered/src/main/java/org/apache/beam/sdk/extensions/ordered/OrderedEventProcessor.java
Outdated
Show resolved
Hide resolved
...ions/ordered/src/main/java/org/apache/beam/sdk/extensions/ordered/OrderedEventProcessor.java
Outdated
Show resolved
Hide resolved
...ions/ordered/src/main/java/org/apache/beam/sdk/extensions/ordered/OrderedEventProcessor.java
Outdated
Show resolved
Hide resolved
...ions/ordered/src/main/java/org/apache/beam/sdk/extensions/ordered/OrderedEventProcessor.java
Outdated
Show resolved
Hide resolved
...ions/ordered/src/main/java/org/apache/beam/sdk/extensions/ordered/OrderedEventProcessor.java
Outdated
Show resolved
Hide resolved
...s/ordered/src/main/java/org/apache/beam/sdk/extensions/ordered/OrderedProcessingHandler.java
Show resolved
Hide resolved
...ns/ordered/src/main/java/org/apache/beam/sdk/extensions/ordered/OrderedProcessingStatus.java
Show resolved
Hide resolved
damccorm
left a comment
damccorm
left a comment
There was a problem hiding this comment.
Thanks! This generally LGTM, I think it would be good to add a few of the tests mentioned in comments/TODOs, otherwise I think the core of the PR looks good to me.
…DO's captured as Beam's issues.
|
I think I addressed all the outstanding issues. PLMK if anything else is missing. |
damccorm
left a comment
damccorm
left a comment
There was a problem hiding this comment.
Thanks! This LGTM, could you move it out of draft? Then I can merge
| KV.of( | ||
| currentSequence, | ||
| UnprocessedEvent.create( | ||
| currentEvent, Reason.sequence_id_outside_valid_range)))); |
There was a problem hiding this comment.
I like this solution, thanks
|
I added extra handling for checked exceptions (outputting the elements which cause them into the DLQ). There is some oddness with one test, but I can address it a bit later (requires new method in PAssert), I will do it a bit later. Changing the PR from Draft to allow for the merge. |
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
* Initial check-in of the ordered processing extension in Java. * Address PR comments. * Address PR comments. * Added JavaDocs to OrderedProcessingStatus.java * Added batch tests. Added DLQ for events with the sequence outside of the valid range. * Added tests for windowed input. Added references to the unresolved TODO's captured as Beam's issues. * Added DLQ handling of checked exceptions happening during the state mutations.
2 participants
Initial PR for the ordered processing in Java. This PR contains all the code needed for ordered processing. There will be another PR to add documentation and code samples. This work was discussed in this design doc.