-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add a CloudConnectionPool #4005
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
Conversation
…ettings constructors to ease connecting to Elastic Cloud
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments, mainly about documentation.
* [[cloud-connection-pool]] | ||
* ==== CloudConnectionPool | ||
* | ||
* A specialized subclass of `SingleNodeConnectionPool that accepts a Cloud Id and credentials. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link to Cloud docs for Cloud Id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't use {ref_current}
for https://www.elastic.co/guide/en/cloud/current/ec-cloud-id.html would need to add a new block I think.
Is my assumption correct @russcam ?
* ==== CloudConnectionPool | ||
* | ||
* A specialized subclass of `SingleNodeConnectionPool that accepts a Cloud Id and credentials. | ||
* When used the client will also pick Elastic Cloud optmized defaults for the connection settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link to Elastic Cloud
src/Tests/Tests/ClientConcepts/ConnectionPooling/BuildingBlocks/ConnectionPooling.doc.cs
Outdated
Show resolved
Hide resolved
*/ | ||
var credentials = new BasicAuthenticationCredentials("username", "password"); | ||
var pool = new CloudConnectionPool(cloudId, credentials); | ||
pool.UsingSsl.Should().BeTrue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should start steering away from emitting test assertions in documentation. I think it adds noise to the signal we're trying to display to users, in this case, how to configure the client for user with Elasticsearch Service on Elastic Cloud. We can prefix and assertions with // hide
, and introduce a block scope {}
and prefix, if needing to hide more.
public BasicAuthenticationCredentials() | ||
{ | ||
} | ||
//TODO remove this constructor in 8.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
{ | ||
} | ||
//TODO remove this constructor in 8.0 | ||
public ApiKeyAuthenticationCredentials() { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
Co-Authored-By: Russ Cam <russ.cam@elastic.co>
Merging this in, will follow up with linking to the cloud id docs. |
* Add CloudConnectionPool and overloads to the high/low level clients settings constructors to ease connecting to Elastic Cloud * Add tests for the CloudConnectionPool * Apply suggestions from code review Co-Authored-By: Russ Cam <russ.cam@elastic.co> * fix failing tests due to updated exception message (cherry picked from commit fdb1933)
* Add CloudConnectionPool and overloads to the high/low level clients settings constructors to ease connecting to Elastic Cloud * Add tests for the CloudConnectionPool * Apply suggestions from code review Co-Authored-By: Russ Cam <russ.cam@elastic.co> * fix failing tests due to updated exception message (cherry picked from commit fdb1933) (cherry picked from commit fd5716f)
This PR allows Elastic Cloud users to use their Cloud Id to configure the client much like they would Logstash / Beats see: https://www.elastic.co/guide/en/cloud/current/ec-cloud-id.html
This also allows us to set some specific
ConnectionSettings
defaults.