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

Reconnect using all options #350

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aleclarson
Copy link
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
Copy link
Owner

neumino commented Sep 3, 2017

Left a few comments, it seems reasonable beside that

@aleclarson
Copy link
Contributor Author

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

@Extarys
Copy link

Extarys commented Sep 12, 2017

@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
Copy link
Contributor Author

aleclarson commented Sep 12, 2017

@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
Copy link

Extarys commented Sep 13, 2017

@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

merged in https://github.com/RebirthDB/rebirthdb-js

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