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.

Sign up for GitHub

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

avoid creating nats stream if stream is passed as empty #1074

Closed
wants to merge 1 commit into from
Closed

avoid creating nats stream if stream is passed as empty #1074

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions protocol/nats_jetstream/v2/sender.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,21 +44,28 @@ func NewSender(url, stream, subject string, natsOpts []nats.Option, jsmOpts []na

// NewSenderFromConn creates a new protocol.Sender which leaves responsibility for opening and closing the NATS
// connection to the caller
// The stream parameter is only needed if auto-creation of the stream is needed when the stream does not exist.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The stream parameter is only needed if auto-creation of the stream is needed when the stream does not exist.
// The stream parameter is optional. If specified, a stream with default stream parameters will be created. If that stream already exists with non-default parameters, creation will fail with an error message.

func NewSenderFromConn(conn *nats.Conn, stream, subject string, jsmOpts []nats.JSOpt, opts ...SenderOption) (*Sender, error) {
jsm, err := conn.JetStream(jsmOpts...)
if err != nil {
return nil, err
}

streamInfo, err := jsm.StreamInfo(stream, jsmOpts...)

if streamInfo == nil || err != nil && err.Error() == "stream not found" {
_, err = jsm.AddStream(&nats.StreamConfig{
Name: stream,
Subjects: []string{stream + ".*"},
})
if err != nil {
return nil, err
// A Stream parameter is not needed for a send operation.
// A subject is all that is needed.
// Below, a stream is created with default stream config which may not be desired.
Copy link
Member

Choose a reason for hiding this comment

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

The wording is a bit misleading here IMHO. IIUC, NATS will only fail if a stream exists but different options are supplied, correct? I.e., the operation is not always idempotent.

Suggested change
// Below, a stream is created with default stream config which may not be desired.
// If a stream name is supplied, an attempt is made to create that stream with default parameters. If that stream does not exist or exists with the same parameters specified in the below call, the operation will succeed. Otherwise an error is returned.

// It may be that the intention is for the call to fail if a stream does not exist.
if stream != "" {
Copy link

Choose a reason for hiding this comment

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

I wonder if a breaking change to add this as an CreateStream Option or similar might be better?

If we're comitted to keeping the parameter we need to update the function documenation to explain 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.

Modifed parameter documentation to include the fact the stream is not required.

streamInfo, err := jsm.StreamInfo(stream, jsmOpts...)

if streamInfo == nil || err != nil && err.Error() == "stream not found" {
Copy link
Member

Choose a reason for hiding this comment

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

I know you're reusing the existing implementation here, but couple questions:

  1. Could there be a scenario where both, streamInfo and err are nil? If not, then just checking the error might be enough here
  2. Instead of asserting on err.Error() please use typed error checking ErrStreamNotFound

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the source, it looks like there is only one line that returns an info object. All other lines return a non-nil error. Once again, I only added an if statement around the existing code. I can make that change if you like. Also, if I were to embark on a new implementation based on the new jetstream api, I might simply make changes there.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, probably better to fix in the other PR when using the new JetStream API. I was just bringing those concerns up here because I wasn't part of the initial reviews and by asking these questions we have opportunities to simplify the code base/not complicate it further for our future selves :)

Appreciate your work!

_, err = jsm.AddStream(&nats.StreamConfig{
Name: stream,
Subjects: []string{stream + ".*"},
})
if err != nil {
return nil, err
}
}
}

Expand Down