Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Ordered Processing PTransform to Java SDK #30735

Merged
merged 8 commits into from
Mar 29, 2024

Conversation

slilichenko
Copy link
Contributor

@slilichenko slilichenko commented Mar 25, 2024

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.

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@slilichenko
Copy link
Contributor Author

I think I addressed all the outstanding issues. PLMK if anything else is missing.

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this solution, thanks

@slilichenko
Copy link
Contributor Author

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.

@slilichenko slilichenko marked this pull request as ready for review March 29, 2024 05:46
Copy link
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@damccorm damccorm merged commit fe2a224 into apache:master Mar 29, 2024
26 checks passed
hjtran pushed a commit to hjtran/beam that referenced this pull request Apr 4, 2024
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants