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 support for reactive Elasticsearch healthcheck #21042

Conversation

aleksanderlech
Copy link

@aleksanderlech aleksanderlech commented Apr 20, 2020

Hello,

I am working a lot with spring-data-elasticsearch using reactive repositories. I noticed that even the newest version still supports only healthchecks for ReactiveElasticsearchClient. More over the default healthcheck still wants to run even if all of the clients are reactive - it uses the default one created by autoconfiguration that fails.

I checked how it was done for Mongo that supports reactive indicators as well as basing on the existing ElasticSearch health indicator I implemented a simple proposal. The generated output would be as follows:

"elasticsearch": {
            "status": "UP",
            "components": {
                "reactiveElasticsearchClient": {
                    "status": "UP",
                    "details": {
                        "192.168.78.150": "ONLINE",
                        "192.168.78.160": "OFFLINE"
                    }
                }
            }
        }

This is my first contribution to the project so please forgive me any mistakes, I tried my best to follow the guidelines. Any comments appreciated. Thank you.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 20, 2020
@aleksanderlech aleksanderlech force-pushed the feature/reactive-elasticsearch-healthcheck-proposal branch from 3202327 to 2bd5b1a Compare April 20, 2020 16:49
@aleksanderlech aleksanderlech force-pushed the feature/reactive-elasticsearch-healthcheck-proposal branch from 2bd5b1a to 21af08b Compare April 20, 2020 17:01
@aleksanderlech aleksanderlech marked this pull request as ready for review April 20, 2020 17:04
@aleksanderlech
Copy link
Author

aleksanderlech commented Apr 20, 2020

any ideas why CI failed? Running locally succeeds 😶

gradle clean --no-build-cache :spring-boot-project:spring-boot-actuator-autoconfigure:test

@philwebb
Copy link
Member

@aleksanderlech Those failures look unrelated to your changes. We've had some issues with diskspace on the CI box, so it might be that.

@aleksanderlech
Copy link
Author

Hello, any feedback here? :)

@bclozel bclozel self-assigned this May 7, 2020
@bclozel
Copy link
Member

bclozel commented May 7, 2020

Thanks for your contribution!
I believe we can't merge this right now as there are several questions we need to answer first.

I agree that we should align here with what we're doing with MongoDB (reactive vs. non-reactive). This changes the name of the contributor beans, which is a breaking change. We should consider that in a minor release (at least), but we're already quite late in the 2.3 cycle with RC1 already being released.

Also, it bothers me that the health response for the reactive case is very different from the one with the base REST client. Your proposal shows a global "UP"/"DOWN" status and specific "ONLINE"/"OFFLINE" node details. The existing indicator queries the "/_cluster/health/" endpoint and reports the health of the cluster itself ("UP", "OUT_OF_SERVICE", "DOWN") with the actual JSON response as details. I'd rather try and align both implementations - I think the actuator health indicator should provide information about the cluster health, rather than the online/offline status of existing nodes.

@christophstrobl and @mp911de might disagree with that point, since the client interface only exposes a client.status() which does not seem to do the same thing. Is there a way to get the cluster health with a ReactiveElasticsearchClient? Should we reconsider our approach for this actuator endpoint?

One last bit: it seems that we've let a typo sneak in (and your contribution aligns there, so not your fault!) - it should be ElasticsearchRestHealthContributorAutoConfiguration and not ElasticSearchRestHealthContributorAutoConfiguration, same for ElasticsearchReactiveHealthContributorAutoConfiguration vs. ElasticSearchReactiveHealthContributorAutoConfiguration.

@mp911de
Copy link
Member

mp911de commented May 7, 2020

There's currently no method to issue a request to /_cluster/health/. However, you can interact with the underlying WebClient through

client.execute(webClient -> webClient.get().uri("/_cluster/health/").exchange()).flatMap(clientResponse -> clientResponse.bodyToMono(…))

to query the cluster state.

It would make sense to provide such a method on ReactiveElasticsearchClient to close the feature gap between ReactiveElasticsearchClient and RestHighLevelClient. /cc @sothawo

@sothawo
Copy link
Contributor

sothawo commented May 7, 2020

@mp911de you mean to to add a healthcheck method to ReactiveElasticsearchClient?
Would it even make sense to have something like ClusterOperations that implement (not all of) ES Cluster API?

@mp911de
Copy link
Member

mp911de commented May 7, 2020

I think so. Let’s continue the cluster API discussion in a DATAES ticket.

@aleksanderlech
Copy link
Author

aleksanderlech commented May 10, 2020

@bclozel thanks for the feedback, I did not expect it to be merged straight away and actually had the same concerns regarding the cluster state and response model but wanted to get some feedback from the team first. I would be more very happy if I can implement it following your suggestions.

@bclozel
Copy link
Member

bclozel commented Jun 5, 2020

@bclozel bclozel added status: blocked An issue that's blocked on an external project change type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 11, 2020
@bclozel bclozel added this to the 2.x milestone Jun 11, 2020
@bclozel bclozel added the for: team-attention An issue we'd like other members of the team to review label Jun 18, 2020
@philwebb philwebb modified the milestones: 2.x, 2.3.x Jun 22, 2020
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Jun 22, 2020
@bclozel bclozel removed the status: blocked An issue that's blocked on an external project change label Jun 22, 2020
bclozel pushed a commit that referenced this pull request Jul 3, 2020
Prior to this commit, configuring a reactive Elasticsearch client would
auto-configure an Actuator Health check using a synchronous client, with
the default configuration properties (so tarting localhost:9200).

This would lead to false reports of unhealthy Elasticsearch clusters
when using reactive clients.

This commit reproduces the logic for MongoDB repositories: if a reactive
variant is available, it is selected for the health check
infrastructure.

See gh-21042
bclozel added a commit that referenced this pull request Jul 3, 2020
@bclozel bclozel changed the title Added support for healthcheck for ReactiveElasticsearchClient Add support for reactive Elasticsearch healthcheck Jul 3, 2020
@bclozel bclozel removed this from the 2.3.x milestone Jul 3, 2020
@bclozel
Copy link
Member

bclozel commented Jul 3, 2020

Merged with 86d8366
Thanks @aleksanderlech !

@bclozel bclozel closed this Jul 3, 2020
@aleksanderlech aleksanderlech deleted the feature/reactive-elasticsearch-healthcheck-proposal branch November 17, 2020 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants