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 possibility to disable the Elasticsearch Sniffer functionality #26949

Conversation

filiphr
Copy link
Contributor

@filiphr filiphr commented Jun 17, 2021

With #24174 out-of-the-box support was added for Elasticsearch Sniffer.

After our upgrade to the latest Spring Boot 2.5.1, we noticed that using Elasticsearch Sniffer with Elasticsearch in Docker was not working correctly (at least not on Mac). The resolved nodes had some IP that could not be accessed from my host. I know that the functionality is there only if the Sniffer is on the classpath (it was for us by mistake).

In my opinion it could be beneficial to enable / disable the functionality using a property as well. This can be interested for application build on top of Spring Boot that would like to offer an opt in to the Sniffer by enabling the flag.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 17, 2021
@filiphr filiphr force-pushed the allow-disabling-elasticsearch-sniffer branch from 0d91b24 to bf04333 Compare June 17, 2021 20:27
@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Jun 17, 2021
@philwebb
Copy link
Member

Looking at the auto-configuration it doesn’t seem like there’s currently an easy way to exclude the sniffer whilst keeping everything else. I think another way to disable it would be good. That could be a property, or we could extract a new auto-configuration class.

@filiphr
Copy link
Contributor Author

filiphr commented Jun 18, 2021

Using a different AutoConfiguration class would work as well. We already have a mechanism that uses AutoConfigurationImportFilter to enable / disable certain auto configurations based on properties. Of course having a simple property directly from Spring Boot is the easiest.

I created this PR since it was easy to do it and I thought I should just do it and we can discuss in the PR, without the need to go through issues.

@philwebb
Copy link
Member

We discussed this today as team and we'd currently prefer to leave things as they are. It feels like having and Elasticsearch Sniffer dependency and not enabling it shouldn't be that common. We've also been trying to reduce the number of .enabled properties.

If more people comment on this issue, or we find a good reason why the sniffer dependency needs to be present but not used, we can reconsider.

@philwebb philwebb closed this Jun 23, 2021
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Jun 23, 2021
@philwebb
Copy link
Member

Regardless, thanks a lot for the PR @filiphr!

@carlosrodfern
Copy link

carlosrodfern commented Jan 7, 2022

We are having the same issue after updating Spring. We use sniffer in our testing and production environments, however, our developers have a shared environment they connect their u-service to to be able to access a large amount of testing data. This environment is behind an ingress proxy, so developers used to have the sniffer off. After updating spring boot, we are having the issue described in the original post.

It would be great to be able to separate the sniffer mechanism from the policy of enabling it or not. The configuration proposed here would be the best approach. Preventing the sniffer from showing up in the classpath in local environments and not in others is not a feasible option.

@filiphr filiphr deleted the allow-disabling-elasticsearch-sniffer branch January 8, 2022 11:01
@filiphr
Copy link
Contributor Author

filiphr commented Jan 8, 2022

@carofe82 there is a way to achieve what you are looking for without the need for a change in Spring Boot. It is a nasty and in my opinion a hacky approach but it it works. Have a look at #28496 (comment) for a hint for how to achieve it.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Jan 9, 2022
@carlosrodfern
Copy link

@filiphr ,
Thank you for the recommendation. At the end, we have decided to just remove sniffer all together from classpath, and instead use other techniques in Production.

@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Jan 12, 2022
@felipelo
Copy link

felipelo commented Nov 15, 2022

I am not really sure about the right process to comment on a closed ticket, but here is my contribution/reason towards having an enabled key on Elasticsearch Sniffer instead of just checking for the class.

In my project, I have Hibernate Search that brings in org.elasticsearch.client:elasticsearch-rest-client-sniffer dependency and causes ElasticsearchRestClientConfigurations to trigger sniffer, because it contains @ConditionalOnClass(Sniffer.class). However, Hibernate Search has a key to disable the Sniffer via discovery.enabled.

Trying to exclude this dependency, the project starts to fail with NoClassDefFoundError

Caused by: java.lang.NoClassDefFoundError: org/elasticsearch/client/sniff/NodesSniffer
    at org.hibernate.search.backend.elasticsearch.cfg.spi.ElasticsearchBackendSpiSettings$Defaults.<clinit>(ElasticsearchBackendSpiSettings.java:31)
    at org.hibernate.search.backend.elasticsearch.impl.ElasticsearchBackendFactory.<clinit>(ElasticsearchBackendFactory.java:62)
    at org.hibernate.search.backend.elasticsearch.impl.ElasticsearchBeanConfigurer.lambda$configure$0(ElasticsearchBeanConfigurer.java:23)

For now, my workaround is to disable Elasticsearch Health Check, but really would like to see it working.

Hibernate Search property: https://docs.jboss.org/hibernate/stable/search/reference/en-US/html_single/#backend-elasticsearch-configuration-discovery

Thank you.

@wilkinsona wilkinsona added the for: team-meeting An issue we'd like to discuss as a team to make progress label Nov 16, 2022
@bclozel
Copy link
Member

bclozel commented Nov 16, 2022

@felipelo In my opinion, shipping an optional feature as a mandatory compile dependency is not optimal. Maybe that's a change the Hibernate search team could consider?

@felipelo
Copy link

@bclozel Thanks for the quick response. I will try with Hibernate team.

@philwebb philwebb removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants