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

[Go SDK]: Create natsio.Write transform for writing to NATS #29184

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

johannaojeling
Copy link
Contributor

Addresses #29000 and implements a Write transform for writing to NATS.

The Read transform is more involved and will be in a separate PR.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Mention the appropriate issue in your description (for example: 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, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

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)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

@johannaojeling johannaojeling changed the title Create natsio.Write transform for writing to NATS [Go SDK]: Create natsio.Write transform for writing to NATS Oct 29, 2023
@codecov
Copy link

codecov bot commented Oct 29, 2023

Codecov Report

Merging #29184 (78c85ad) into master (c2816c8) will increase coverage by 0.03%.
Report is 92 commits behind head on master.
The diff coverage is 68.05%.

@@            Coverage Diff             @@
##           master   #29184      +/-   ##
==========================================
+ Coverage   38.38%   38.41%   +0.03%     
==========================================
  Files         687      691       +4     
  Lines      101745   101876     +131     
==========================================
+ Hits        39051    39137      +86     
- Misses      61113    61151      +38     
- Partials     1581     1588       +7     
Flag Coverage Δ
go 53.50% <68.05%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
sdks/go/pkg/beam/io/natsio/write_option.go 0.00% <0.00%> (ø)
sdks/go/pkg/beam/io/natsio/write.go 86.95% <86.95%> (ø)
sdks/go/pkg/beam/io/natsio/common.go 40.90% <40.90%> (ø)

... and 12 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@github-actions
Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @riteshghorse for label go.
R: @ahmedabu98 for label io.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

Copy link
Contributor

@riteshghorse riteshghorse left a comment

Choose a reason for hiding this comment

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

Go changes looks fine to me and the rest as well. I noticed the different package name for example_test.go but I don't have strong opinion on that. In fact that makes sense to separate out the test functions.

I would let @ahmedabu98 do a pass for IO side and should be good to merge then!

@johannaojeling
Copy link
Contributor Author

Go changes looks fine to me and the rest as well. I noticed the different package name for example_test.go but I don't have strong opinion on that. In fact that makes sense to separate out the test functions.

Thanks for reviewing! Regarding the <package>_test name, it is the convention for writing testable examples to show up as code examples in the go doc (the user should be guided to use natsio.Write rather than Write)

@riteshghorse
Copy link
Contributor

Go changes looks fine to me and the rest as well. I noticed the different package name for example_test.go but I don't have strong opinion on that. In fact that makes sense to separate out the test functions.

Thanks for reviewing! Regarding the <package>_test name, it is the convention for writing testable examples to show up as code examples in the go doc (the user should be guided to use natsio.Write rather than Write)

that's great! thanks for clarifying!

@ahmedabu98
Copy link
Contributor

@lostluck I'm not really familiar with Go, do you mind taking a look for the IO parts of this PR?

Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

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

LGTM for Go (although Ritesh already did that).

Just the one IO style thing. @ahmedabu98 can confirm if that remains an IO best practice though.

opt(option)
}

beam.ParDo0(s, newWriteFn(uri, option), col)
Copy link
Contributor

Choose a reason for hiding this comment

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

Style-wise, even for sinks/writes, it's best to return a PCollection anyway and emit something when the write has been persisted. This allows composition of writing transforms in a pipeline.

Eg. One could want to do something in the same pipeline after the writes have been made. Having an output allows something downstream to use a side input to block until that Write is finished for a given window.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right, we strongly recommend against using PDone in Java for example. In general, we're leaning towards having transforms output something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated

type ProducerMessage struct {
Subject string
ID string
Headers nats.Header
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This nats.Header is a map[string][]string type which comes with some additional utility methods, similar to http.Header. It just hit me it might be preferable to change

-	Headers nats.Header
+	Headers map[string][]string

to have the user only deal with Beam code and not leak out that of third-party libraries used in the implementation. I recall reading that in the I/O standards.

Thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to follow the standards

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this change wasn't pushed? We still have nats.Header there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was pushed in 78c85ad so should be visible if refreshing the files

Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you kindly. Nats is neat.

@lostluck lostluck merged commit cf560ca into apache:master Nov 6, 2023
10 checks passed
@johannaojeling johannaojeling deleted the feat/natsio-write branch November 7, 2023 17:44
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.

4 participants