[ADDED] NatsOptions to configure the NATS connection on stan.Connect() #355
Conversation
This allows users that don't want to create their own NATS connection to configured as desired, to pass the NATS options they would use to create the NATS connection. With that, the streaming connection will create and own the NATS connection but will be configured based on the provided NATS options, except for some options that are overridden by the library. Resolve #354 Signed-off-by: Ivan Kozlovic <[email protected]>
| @@ -357,15 +372,22 @@ func Connect(stanClusterID, clientID string, options ...Option) (Conn, error) { | |||
| c.nc = c.opts.NatsConn | |||
| // Create a NATS connection if it doesn't exist. | |||
| if c.nc == nil { | |||
| nopts := c.opts.NatsOptions | |||
| nopts = append(nopts, nats.MaxReconnects(-1), nats.ReconnectBufSize(-1)) | |||
There was a problem hiding this comment.
Torn between doing that (overriding possibly specified user NATS options), and doing: if user provided NATS options, use as-is, otherwise, do what was done before (create NATS connection with Name(), MaxReconnects() and ReconnectBufSize() options).
There was a problem hiding this comment.
I guess, it contradicts to logic when user provides not nil c.opt.NatsConn since it also might has wrong nats.MaxReconnects and nats.ReconnectBufSize params. Maybe now with NatsOptions introduced it's reasonable to remove/deprecatefunc NatsConn(nc *nats.Conn) Option?
There was a problem hiding this comment.
Are you saying that when user provides NATS connection, the library does not override anything, so if the user were to provide the options (with this new NatsOptions() call), then there is no reason to override either?
As for removing, probably not, but we could add a "deprecated" or mention that users should favor configuring the NATS connection using NatsOptions() instead of providing their own).
There was a problem hiding this comment.
Are you saying that when user provides NATS connection, the library does not override anything, so if the user were to provide the options (with this new NatsOptions() call), then there is no reason to override either?
Yes, that's what I mean. If these options are so crucial, it will be better to forbid to overwrite them imo.
There was a problem hiding this comment.
I added a deprecation note on NatsConn().
Signed-off-by: Ivan Kozlovic <[email protected]>
This allows users that don't want to create their own NATS connection
to configured as desired, to pass the NATS options they would use
to create the NATS connection.
With that, the streaming connection will create and own the NATS
connection but will be configured based on the provided NATS options,
except for some options that are overridden by the library.
Resolve #354
Signed-off-by: Ivan Kozlovic [email protected]