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

Add cloudId builder to the HLRC #47868

Merged
merged 4 commits into from
Oct 14, 2019
Merged

Add cloudId builder to the HLRC #47868

merged 4 commits into from
Oct 14, 2019

Conversation

hub-cap
Copy link
Contributor

@hub-cap hub-cap commented Oct 10, 2019

Elastic cloud has a concept of a cloud Id. This Id is a base64 encoded
url, split up into a few parts. This commit allows the user to pass in a
cloud id now, which is translated to a HttpHost that is defined by the
encoded parts therein.

Elastic cloud has a concept of a cloud Id. This Id is a base64 encoded
url, split up into a few parts. This commit allows the user to pass in a
cloud id now, which is translated to a HttpHost that is defined by the
encoded parts therein.
@hub-cap hub-cap added >enhancement :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch v8.0.0 v7.5.0 labels Oct 10, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Java Low Level REST Client)

@hub-cap
Copy link
Contributor Author

hub-cap commented Oct 10, 2019

@elasticmachine run elasticsearch-ci/1

@hub-cap
Copy link
Contributor Author

hub-cap commented Oct 10, 2019

@elasticmachine run elasticsearch-ci/default-distro

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM, I left some suggestions but they're up to you!

// once decoded the parts are separated by a $ character
String[] decodedParts = decoded.split("\\$");
if (decodedParts.length != 3) {
throw new IllegalStateException("cloudId " + cloudId + " did not contain the correct number of parts");
Copy link
Member

Choose a reason for hiding this comment

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

This error will be really confusing for users who have no idea that cloudIds contain multiple parts, or that they are even encoded at all. Maybe we can change it to something like:

"clouldId ... is invalid and does not decode to a cluster identifier correctly"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well the cloudID is masked for the user. It would be a copy/paste issue since these are generated and displayed in the UI for a user. But i can change it for sure.

@@ -119,6 +120,32 @@
setNodes(nodes);
}

/**
* Returns a new {@link RestClientBuilder} to help with {@link RestClient} creation.
* Creates a new builder instance and sets the nodes that the client will send requests to.
Copy link
Member

Choose a reason for hiding this comment

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

I think adding an example of what a cloudId looks like to the javadoc would help for people who don't know what that is.

// there is an optional first portion of the cloudId that is a human readable string, but it is not used.
if (cloudId.contains(":")) {
if (cloudId.indexOf(":") == cloudId.length() - 1) {
throw new IllegalStateException("cloudId " + cloudId + " is invalid");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new IllegalStateException("cloudId " + cloudId + " is invalid");
throw new IllegalStateException("cloudId " + cloudId + " is invalid, it cannot end with a ':'");

@hub-cap hub-cap merged commit 0fb02aa into elastic:master Oct 14, 2019
@hub-cap hub-cap deleted the hlrc_cloudid branch October 14, 2019 17:46
hub-cap added a commit that referenced this pull request Oct 14, 2019
Elastic cloud has a concept of a cloud Id. This Id is a base64 encoded
url, split up into a few parts. This commit allows the user to pass in a
cloud id now, which is translated to a HttpHost that is defined by the
encoded parts therein.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants