Skip to content

Reconnect using all options - #350

Open
aleclarson wants to merge 1 commit into
neumino:masterfrom
aleclarson:reconnect
Open

Reconnect using all options#350
aleclarson wants to merge 1 commit into
neumino:masterfrom
aleclarson:reconnect

Conversation

@aleclarson

Copy link
Copy Markdown
Contributor

Throw an error if options.connection is defined.
Retain the options for subsequent calls to r.connect when trying to reconnect.

The following options were not being used in reconnections:

  • username
  • password
  • pingInterval
  • timeout
  • ssl

Throw an error if `options.connection` is defined.
Retain the `options` for subsequent calls to `r.connect` when trying to reconnect.
The following options were not being used in reconnections:
  - username
  - password
  - pingInterval
  - timeout
  - ssl
@neumino

neumino commented Sep 3, 2017

Copy link
Copy Markdown
Owner

Left a few comments, it seems reasonable beside that

@aleclarson

Copy link
Copy Markdown
Contributor Author

@neumino I can't find where you left the comments 😅

@Extarys

Extarys commented Sep 12, 2017

Copy link
Copy Markdown

@aleclarson I think he meant that you comment a little before the merge can happen.

Also, I noticed that there is a lot more options than the one being passed, like tls and connection pools, shouldn't we add those or it will reconnect with those settings by it's own?

@aleclarson

aleclarson commented Sep 12, 2017

Copy link
Copy Markdown
Contributor Author

@Extarys Not much to add comments to, it's just 3 changes and 1 addition. And there are already 2 comments.

The connection pool manages each connection. The connection has no knowledge of belonging to a connection pool. Thus, no pool-specific options exist in this context.

As for TLS, that configuration is done using the options.ssl object, which is already accounted for.

@Extarys

Extarys commented Sep 13, 2017

Copy link
Copy Markdown

@aleclarson That's why I wasn't sure if it was needed. I mean you guys are the pros I'm just a guy using the software :P
Thanks for taking the time to clarify this for me, I feel more intelligent already

@thelinuxlich

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants