-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Go SDK]: Implement natsio.Read transform for reading from NATS #29410
Conversation
R: @lostluck I have taken inspiration from unbounded sources/SDFs in the Java SDK (KafkaIO, PulsarIO, etc.) and have tried to keep it simple in this first iteration. It is the first streaming connector I write so let me know if there are any best practices I'm missing out on. |
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #29410 +/- ##
==========================================
- Coverage 37.99% 37.95% -0.05%
==========================================
Files 691 695 +4
Lines 101309 101499 +190
==========================================
+ Hits 38497 38520 +23
- Misses 61213 61366 +153
- Partials 1599 1613 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thank you for your patience for the delay in reviewing. I've been distracted with chasing flakes with Prism, and have also fallen sick. It's going to take a little longer. |
No worries, hope you feel better soon! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience!
As always, your code remains a delight to read. I don't have much to add, as AFAICT you're using all the parts correctly. The trick is whether different runners split in a reasonable fashion (specifically Prism, which will probably be very aggressive).
_ []byte, | ||
rest offsetrange.Restriction, | ||
) []offsetrange.Restriction { | ||
return []offsetrange.Restriction{rest} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so this means no initial splits, so a single stream to start, and only dynamic splits will occur. I think this is correct WRT the rest of the code.
daf959d
to
8af3489
Compare
Thank you for reviewing! I resolved the conflict in CHANGES.md, so if you are happy it should be good to merge. It was fun to learn more about unbounded sources 😊 |
Looks like there's another conflict in CHANGES.md unfortunately, @johannaojeling would you mind resolving again? @lostluck is this good to merge otherwise? |
8af3489
to
b49d27f
Compare
Thank you for your patience WRT merging it in. Thanks Danny for the reminder! |
Implements a
natsio.Read
transform for reading from NATS JetStream. Fixes #29000.Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.