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

Implement connection string #49

Merged
merged 6 commits into from
Jul 8, 2020
Merged

Implement connection string #49

merged 6 commits into from
Jul 8, 2020

Conversation

shaan1337
Copy link
Member

@shaan1337 shaan1337 commented Jun 26, 2020

Fixes #41

Note: This PR has been opened against use-https-option branch so that it's retargeted to master after the former is merged.

Examples:

single node:
EventStoreClientSettings.Create("esdb://localhost"); //with hostname only
EventStoreClientSettings.Create("esdb://localhost:2113"); //with hostname and port
EventStoreClientSettings.Create("esdb://user:pass@localhost:2113"); //with username and password
EventStoreClientSettings.Create("esdb://user:pass@localhost:2113/"); //extra forward slash also works
EventStoreClientSettings.Create("esdb://user:pass@localhost:2113?tlsVerifyCert=false"); //disables server certificate validation
EventStoreClientSettings.Create("esdb://user:pass@localhost:2113?tls=false"); //connects over http instead of https

cluster:
EventStoreClientSettings.Create("esdb://host1,host2,host3"); //with hostnames only
EventStoreClientSettings.Create("esdb://host1:1234,host2:4321,host3:3231"); //with hostnames and port
EventStoreClientSettings.Create("esdb://user:pass@host1:1234,host2:4321,host3:3231?nodePreference=follower"); //with username and password
EventStoreClientSettings.Create("esdb://host1,host2,host3?tls=false"); //connects over http instead of https
EventStoreClientSettings.Create("esdb://host1,host2,host3?tlsVerifyCert=false"); //disables server certificate validation

All parameters:

ConnectionName / string
MaxDiscoverAttempts / int
DiscoveryInterval / int (milliseconds)
GossipTimeout / int (milliseconds)
NodePreference / string
Tls / bool
TlsVerifyCert / bool
OperationTimeout / int (milliseconds)
ThrowOnAppendFailure / bool

@shaan1337 shaan1337 marked this pull request as draft June 26, 2020 12:27
@shaan1337 shaan1337 added this to the EventStoreDB 20.6.1 milestone Jun 29, 2020
@shaan1337 shaan1337 force-pushed the connection-string branch 3 times, most recently from 773c848 to 5b1bc02 Compare July 1, 2020 07:24
@shaan1337 shaan1337 changed the base branch from master to use-https-option July 1, 2020 07:37
@shaan1337 shaan1337 marked this pull request as ready for review July 1, 2020 07:44
Base automatically changed from use-https-option to master July 1, 2020 12:42
@shaan1337 shaan1337 force-pushed the connection-string branch from 5b1bc02 to ea3cda7 Compare July 1, 2020 12:50
@shaan1337 shaan1337 requested a review from thefringeninja July 2, 2020 05:22
@shaan1337 shaan1337 dismissed thefringeninja’s stale review July 2, 2020 05:23

changes have been made as requested

thefringeninja
thefringeninja previously approved these changes Jul 2, 2020
@pgermishuys
Copy link
Contributor

Let's try and marry up the units to that of the server.

thefringeninja
thefringeninja previously approved these changes Jul 2, 2020
Copy link
Member

@hayley-jean hayley-jean left a comment

Choose a reason for hiding this comment

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

Just two things I've found during testing:

  1. It's possible to provide duplicate values in the connection string. This results in the last one specified taking effect, should we leave this or should this cause an error?
  2. Perhaps there should be some validation checking for the ? after specifying the host? There's no error if the ? is omitted, but the settings don't take effect.

@shaan1337
Copy link
Member Author

@hayley-jean good catch, thanks for the feedback, I've addressed these issues in these 2 commits:
f4ffa9c
bfad201

@shaan1337 shaan1337 requested a review from hayley-jean July 6, 2020 14:46
@pgermishuys
Copy link
Contributor

Hey @shaan1337, good work on this!
Is there any particular reason why we are requiring a / and ? after the hosts to indicate extra parameters? Wouldn't a single character such as a ; give it a better user experience?

@shaan1337
Copy link
Member Author

@pgermishuys yes, the connection string mimics a URL as much as possible and ? is part of the syntax: https://en.wikipedia.org/wiki/URL#Syntax

Right now we enforce a ? just after the / but the path could later on point to a specific resource, e.g if we later on support multi-tenancy we could specify the database name in the path:
EventStoreClientSettings.Create("esdb://localhost/mydatabase1?tls=false");

@shaan1337 shaan1337 force-pushed the connection-string branch from e835a46 to 74acd58 Compare July 7, 2020 10:42
@shaan1337 shaan1337 force-pushed the connection-string branch from 74acd58 to 52ffeab Compare July 7, 2020 10:47
@shaan1337
Copy link
Member Author

shaan1337 commented Jul 7, 2020

@pgermishuys @hayley-jean @thefringeninja
I've added a new commit (52ffeab) to make the path optional and removed the last 2 commits. It's up for review again (except that some unrelated flaky test is failing intermittently: EventStore.Client.listing_users.returns_all_users)

@hayley-jean hayley-jean merged commit 88d931e into master Jul 8, 2020
@hayley-jean hayley-jean deleted the connection-string branch July 8, 2020 08:10
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.

Add development mode extension to the client
4 participants