[Go SDK]: Implement natsio.Read transform for reading from NATS#29410
[Go SDK]: Implement natsio.Read transform for reading from NATS#29410lostluck merged 2 commits intoapache:masterfrom
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! |
lostluck
left a comment
There was a problem hiding this comment.
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).
sdks/go/pkg/beam/io/natsio/read.go
Outdated
There was a problem hiding this comment.
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.Readtransform 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.mdwith 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.