avoid creating nats stream if stream is passed as empty #1074
Conversation
| // A subject is all that is needed. | ||
| // Below, a stream is created with default stream config which may not be desired. | ||
| // It may be that the intention is for the call to fail if a stream does not exist. | ||
| if stream != "" { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Modifed parameter documentation to include the fact the stream is not required.
stephen-totty-hpe
force-pushed
the
totty/nats-sender-stream-2024
branch
from
3f4717e
to
58a3880
Compare
| if stream != "" { | ||
| streamInfo, err := jsm.StreamInfo(stream, jsmOpts...) | ||
|
|
||
| if streamInfo == nil || err != nil && err.Error() == "stream not found" { |
There was a problem hiding this comment.
I know you're reusing the existing implementation here, but couple questions:
- Could there be a scenario where both, streamInfo and err are nil? If not, then just checking the error might be enough here
- Instead of asserting on err.Error() please use typed error checking
ErrStreamNotFound
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
| 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. |
There was a problem hiding this comment.
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.
| // 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. |
| @@ -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. | |||
There was a problem hiding this comment.
| // 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. |
|
@stephen-totty-hpe ping |
Signed-off-by: stephen-totty-hpe <[email protected]>
duglin
force-pushed
the
totty/nats-sender-stream-2024
branch
from
58a3880
to
e3c2d38
Compare
I will close this PR if #1095 gets merged. |
A stream name is not needed when sending a NATS message. The subject should be enough to send a NATS message. Currently, a stream is required only because a stream is being used to "auto-create" a stream.
In many cases: