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

Added overload to Raven Health check to allow user to pass RavenDBOptions #139

Merged
merged 1 commit into from Apr 4, 2019

Conversation

Projects
None yet
2 participants
@ankitvijay
Copy link

commented Apr 2, 2019

With this pull request, I have added a method overload to allow user to pass array of Raven URLs RavenDBOptions instead of just single connection string. Note that existing method which allows the user to pass single connection string is still there but it has been marked Obsolete.

Why this change?

  • This change is useful to test the health check for the Raven DB Cluster (that is multiple nodes) instead of a single connection string. For Load Balance and Fail over scenarios we would have multiple connection strings instead of one.
  • In addition to this, RavenDBOptions also supports certificate authentication. That is user can optionally pass the authentication certificate, which should ideally be a requirement for any production workload (like ours)

As an alternate (without the changes in this PR), we can add multiple raven DB urls as pseudocode below:

    var counter = 1;
    foreach(var url in urls)
    {
        healthCheckBuilder.AddRavenDB(url, "DemoDbName",$"ravendb node{counter}, url: {url} ");
        ++counter;
    }

However, this is less than ideal for following reasons:

  • It means additional lines of code.
  • If one of urls is down/ unavailable the overall health status would be returned as "Unhealthy" which is incorrect since server can still connect to the alternate Raven DB url.
  • In scenarios where authentication is required we cannot use this health check library.
@unaizorrilla

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2019

Hi @ankitvijay

I try to review this ASAP!

@unaizorrilla
Copy link
Collaborator

left a comment

Hi,
@ankitvijay sounds good. I summitted some minor changes, can you address this?

@ankitvijay ankitvijay force-pushed the ankitvijay:master branch 3 times, most recently from 78413ed to 3ae076e Apr 2, 2019

@ankitvijay ankitvijay changed the title Added overload to Raven Health check to allow array of raven Urls Added overload to Raven Health check to allow user to pass `RavenDBOptions` Apr 2, 2019

@ankitvijay ankitvijay changed the title Added overload to Raven Health check to allow user to pass `RavenDBOptions` Added overload to Raven Health check to allow user to pass RavenDBOptions Apr 2, 2019

vijayankit
- Added a method overload to Raven Health check to allow additional R…
…aven Options like array of Urls, certificate etc.

- Marked the existing Raven Health check method as `Obsolete`

@ankitvijay ankitvijay force-pushed the ankitvijay:master branch from 3ae076e to ee3adfc Apr 3, 2019

@ankitvijay

This comment has been minimized.

Copy link
Author

commented Apr 3, 2019

Hi @unaizorrilla when you get a chance could you look into my PR please. I'm waiting for it to merge to start using it my project. Thanks!

@unaizorrilla

This comment has been minimized.

Copy link
Collaborator

commented Apr 3, 2019

Yeap, tomorrow!

@unaizorrilla unaizorrilla merged commit ee3adfc into Xabaril:master Apr 4, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@unaizorrilla

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

Merged, thanks for contributing. Build Release is on appveyor, when finish a new package 2.2.2. for RavenDb will be on NuGet.

@ankitvijay

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

Thanks @unaizorrilla

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.