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

adding actuator health check support for Elasticsearch REST Clients #15211

Closed

Conversation

Projects
None yet
5 participants
@ayudovin
Copy link
Contributor

commented Nov 19, 2018

@ayudovin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

Frankly speaking unit tests are blocked me now because I can't find a good way for writing unit tests. Can somebody give me advice?

@wilkinsona

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

IMO, unit testing of a health indicator that interacts with a server in what can be a reasonably complex way is of limited benefit. The danger is that you end up mocking out the server in a way that doesn't match how it behaves. We've seen problems along those lines with Couchbase, for example, where the health indicator was calling something that blocked for an unacceptably long time when a node was down. If there's a Docker image available for Elasticsearch, I'd explore using Testcontainers instead.

@ayudovin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

Thank you for your advice. It makes sense. I'll try to adding unit tests via Testcontainers.

@@ -263,6 +263,11 @@
<artifactId>elasticsearch</artifactId>
<optional>true</optional>
</dependency>
<dependency>
<groupId>org.elasticsearch.client</groupId>
<artifactId>elasticsearch-rest-high-level-client</artifactId>

This comment has been minimized.

Copy link
@bclozel

bclozel Nov 19, 2018

Member

Why elasticsearch-rest-high-level-client? Spring Boot supports both elasticsearch-rest-client and elasticsearch-rest-high-level-client (which is using the former under the covers). So I think we could just have a health indicator for elasticsearch-rest-client and cover both.

This comment has been minimized.

Copy link
@ayudovin

ayudovin Nov 20, 2018

Author Contributor

ok, I've added changes by your suggestion.

*/

@Configuration
@ConditionalOnClass(RestHighLevelClient.class)

This comment has been minimized.

Copy link
@bclozel

bclozel Nov 19, 2018

Member

The RestClientAutoConfiguration is @ConditionalOnClass(RestClient.class), configuring the "low-level" RestClientBuilder in all cases, and the high level one if possible. I think we should just focus on building a low level RestClient using the auto-configured RestClientBuilder.

This comment has been minimized.

Copy link
@ayudovin

ayudovin Nov 20, 2018

Author Contributor

I've added changes by your suggestion.


@Override
protected void doHealthCheck(Health.Builder builder) throws Exception {
MainResponse info = this.client.info(RequestOptions.DEFAULT);

This comment has been minimized.

Copy link
@bclozel

bclozel Nov 19, 2018

Member

The ElasticsearchJestHealthIndicator is using the the health API, which is actually requesting the "/_cluster/health/" endpoint. I think we should align on that here. The RestHighLevelClient doesn't expose such an API, so that's another reason in favor of using the RestClient and manually issuing a request to that endpoint. Ideally, we should derive the health status from the "status" information, just like in ElasticsearchJestHealthIndicator.

This comment has been minimized.

Copy link
@ayudovin

ayudovin Nov 20, 2018

Author Contributor

I've added changes by your suggestion.

ayudovin added some commits Nov 20, 2018

@bclozel bclozel modified the milestones: 2.1.x, 2.1.1 Nov 28, 2018

bclozel added a commit that referenced this pull request Nov 28, 2018

Add Actuator health checks for Elasticsearch REST clients
This commit adds `ElasticsearchRestHealthIndicator`, a new
`HealthIndicator` for Elasticsearch, using the Elasticsearch "low level
rest client" provided by the
`"org.elasticsearch.client:elasticsearch-rest-client"` dependency.

Note that Spring Boot will auto-configure both low and high level REST
clients, but since the high level one is using the former, a single
health indicator will cover both cases.

See gh-15211

@bclozel bclozel closed this in d12e42e Nov 28, 2018

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.