Skip to content

Verify that main info response returns correct product headers #73910

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jun 17, 2021
Merged

Verify that main info response returns correct product headers #73910

merged 9 commits into from
Jun 17, 2021

Conversation

@swallez
Copy link
Contributor

@swallez swallez commented Jun 8, 2021

Follow-up to #73434

Ensures that High Level Rest Client is running against a verified
Elasticsearch. When the first request is sent on HLRC, a request to the
info endpoint is made first to verify the product identification and
version.

@swallez swallez requested a review from dakrone June 8, 2021 15:31
@swallez swallez self-assigned this Jun 8, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jun 8, 2021

Pinging @elastic/es-core-features (Team:Core/Features)

@elasticsearchmachine elasticsearchmachine added the external-contributor Pull request authored by a developer outside the Elasticsearch team label Jun 8, 2021
@swallez
Copy link
Contributor Author

swallez commented Jun 8, 2021

Build failures are because #73434 has not been backported to 7.x (see #73434 (comment)). The code is ready for review though.

@dakrone dakrone requested a review from jbaiera June 8, 2021 17:25
@swallez swallez force-pushed the check-product-compatibility branch from 8cf8b0b to 352f276 Compare June 9, 2021 09:52
dakrone
dakrone requested changes Jun 9, 2021
View reviewed changes
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.

Thanks for working on this @swallez, I left some comments

@dakrone dakrone added :Core/Features/Java High Level REST Client and removed external-contributor Pull request authored by a developer outside the Elasticsearch team labels Jun 10, 2021
dakrone
dakrone approved these changes Jun 10, 2021
View reviewed changes
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, thanks for iterating on this. I left a few other minor nits but nothing major.

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java Outdated
Copy link
Member

@dakrone dakrone Jun 10, 2021

Choose a reason for hiding this comment

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

Can you add a comment about how this essentially acts as a memoized method? Before fully understanding this code I assumed it was doing the version check for every request, rather than once and then return a completed future with an optional string.

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java Outdated
Copy link
Member

@dakrone dakrone Jun 10, 2021

Choose a reason for hiding this comment

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

Suggested change
return Optional.of("Elasticsearch version 6 or more is required");
return Optional.of("Elasticsearch version 6 or higher is required");

jbaiera
jbaiera requested changes Jun 10, 2021
View reviewed changes
Copy link
Member

@jbaiera jbaiera left a comment

Choose a reason for hiding this comment

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

A quick note about compatibility coverage that should be addressed before merge:

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java Outdated
Copy link
Member

@jbaiera jbaiera Jun 10, 2021

Choose a reason for hiding this comment

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

We should perform additional validation here of build_flavor and taglines.

jbaiera
jbaiera reviewed Jun 14, 2021
View reviewed changes
client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java Outdated
Copy link
Member

@jbaiera jbaiera Jun 14, 2021

Choose a reason for hiding this comment

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

I think we want to do the tagline validation here as well.

swallez added 6 commits June 16, 2021 19:19
@swallez
Verify main info response returns correct product headers
283230e 
Follow-up to elastic#73434

Ensures that High Level Rest Client is running against a verified
Elasticsearch. When the first request is send on HLRC, a request to the
info endpoint is made first to verify the product identification and
version.
@swallez
Add comments
657c7d9
@swallez
Consider 401 and 403 as nominal responses for product validation
64c5684
@swallez
Update comment on future cancellation
8f9cf5d
@swallez
Add build flavor and tagline to product compatibility verification
ceb527a
@swallez
Refactor/rename and document cancellation forwarding
115e9d1
@swallez swallez force-pushed the check-product-compatibility branch from ec19ee3 to 115e9d1 Compare June 16, 2021 17:19
@swallez
Copy link
Contributor Author

swallez commented Jun 17, 2021 •
edited
Loading

@dakrone I added some comments and refactored a bit in 115e9d1 and 94d88f7: cancelFlag is now a cancellationForwarder that is effectively cancelled. This should make things clearer.

@jbaiera I added build flavor and tagline validation in ceb527a and b87302d

Can you both please cross-check? Thanks!

dakrone
dakrone requested changes Jun 17, 2021
View reviewed changes
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.

I think we need to support OSS builds here also, so we need to change the build flavor check

jbaiera
jbaiera approved these changes Jun 17, 2021
View reviewed changes
Copy link
Member

@jbaiera jbaiera left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @swallez!

dakrone
dakrone approved these changes Jun 17, 2021
View reviewed changes
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 also, thanks for doing this @swallez!

@swallez
Copy link
Contributor Author

swallez commented Jun 17, 2021 •
edited
Loading

Thank you both for your patience! Now waiting for CI to hopefully be happy and I'll hit that merge button 😉

@swallez swallez merged commit c06608d into elastic:master Jun 17, 2021
@swallez swallez deleted the check-product-compatibility branch June 17, 2021 21:29
@swallez swallez mentioned this pull request Jun 17, 2021
Merged
This was referenced Jun 18, 2021
Closed
Merged
@sskrla sskrla mentioned this pull request Aug 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dakrone dakrone dakrone approved these changes

@jbaiera jbaiera jbaiera approved these changes

Assignees

@swallez swallez

Labels

>enhancement Team:Data Management Meta label for data/management team v7.14.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@swallez @elasticmachine @dakrone @jbaiera @jakelandis @probakowski @elasticsearchmachine