Skip to content

Add support for Elasticsearch Sniffer #24340

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

Closed
wants to merge 4 commits into from
Closed

Add support for Elasticsearch Sniffer #24340

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b7fd39a
Provide out-of-the-box support for Elasticsearch Sniffer #24174
somayaj Dec 5, 2020
5abf542
Fixes per review.
somayaj Dec 21, 2020
9ca5436
Fixes per review.
somayaj Dec 22, 2020
c7932c3
Fixes per review.
somayaj Dec 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions spring-boot-project/spring-boot-autoconfigure/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ dependencies {
optional("org.eclipse.jetty.websocket:javax-websocket-server-impl")
optional("org.ehcache:ehcache")
optional("org.elasticsearch.client:elasticsearch-rest-client")
optional("org.elasticsearch.client:elasticsearch-rest-client-sniffer")
optional("org.elasticsearch.client:elasticsearch-rest-high-level-client")
optional("org.flywaydb:flyway-core")
optional("org.freemarker:freemarker")
Expand Down
17 changes: 17 additions & 0 deletions ...gframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.net.URI;
import java.net.URISyntaxException;
import java.time.Duration;
import java.time.temporal.TemporalUnit;

import org.apache.http.HttpHost;
import org.apache.http.auth.AuthScope;
Expand All @@ -31,13 +32,16 @@
import org.elasticsearch.client.RestClientBuilder;
import org.elasticsearch.client.RestHighLevelClient;

import org.elasticsearch.client.sniff.Sniffer;
import org.elasticsearch.client.sniff.SnifferBuilder;
import org.springframework.beans.factory.ObjectProvider;
import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
import org.springframework.boot.autoconfigure.condition.ConditionalOnClass;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.boot.context.properties.PropertyMapper;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Conditional;
import org.springframework.context.annotation.Configuration;
import org.springframework.util.StringUtils;

Expand Down Expand Up @@ -80,6 +84,19 @@ RestClientBuilder elasticsearchRestClientBuilder(ElasticsearchRestClientProperti
return builder;
}

@Bean
@Conditional(IsSnifferAvailableOnClasspathCondition.class)
Copy link
Member

Choose a reason for hiding this comment

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

That's not how classpath checks work. Can you please have a look to @ConditionalOnClass and how it's used in other parts of this codebase (in particular, it's not added on methods, check the Javadoc for more details).

@ConditionalOnMissingBean(RestHighLevelClient.class)
Sniffer elasticsearchSnifferBuilder(ElasticsearchRestClientProperties properties,
Copy link
Member

Choose a reason for hiding this comment

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

The builder in the name sounds unnecessary.

RestClient elasticSearchRestClient) {
Copy link
Member

Choose a reason for hiding this comment

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

Have you run the test before pushing an update? As I've explained several times in this issue, we do not expose a RestClient so requiring a RestClient bean is never going to work.

return Sniffer.builder(elasticSearchRestClient)
.setSniffIntervalMillis(
Math.toIntExact(properties.getSniffInterval().toMillis()))
.setSniffAfterFailureDelayMillis(
Math.toIntExact(properties.getSniffFailureDelay().toMillis()))
.build();
}

private HttpHost createHttpHost(String uri) {
try {
return createHttpHost(URI.create(uri));
Expand Down
23 changes: 23 additions & 0 deletions ...g/springframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ public class ElasticsearchRestClientProperties {
* Read timeout.
*/
private Duration readTimeout = Duration.ofSeconds(30);
/**
* Sniffer interval
Copy link
Member

Choose a reason for hiding this comment

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

Configuration property description ends with a dot. Please consider reviewing the description to better explain what that does.

*/
private Duration sniffInterval = Duration.ofMinutes(5L);
Copy link
Member

Choose a reason for hiding this comment

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

Those properties should be in a public static class Sniffer inner class. Check other @ConfigurationProperties for an example.

/**
* Sniffer failure delay.
*/
private Duration sniffFailureDelay = Duration.ofMinutes(1L);

public List<String> getUris() {
return this.uris;
Expand Down Expand Up @@ -97,4 +105,19 @@ public void setReadTimeout(Duration readTimeout) {
this.readTimeout = readTimeout;
}

public Duration getSniffInterval() {
return sniffInterval;
Copy link
Member

Choose a reason for hiding this comment

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

missing this.. This sort of things can be easily spot if you ran the build locally before pushing your changes.

}

public void setSniffInterval(Duration sniffInterval) {
this.sniffInterval = sniffInterval;
}

public Duration getSniffFailureDelay() {
return sniffFailureDelay;
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

}

public void setSniffFailureDelay(Duration sniffFailureDelay) {
this.sniffFailureDelay = sniffFailureDelay;
}
}
21 changes: 21 additions & 0 deletions ...ingframework/boot/autoconfigure/elasticsearch/IsSnifferAvailableOnClasspathCondition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package org.springframework.boot.autoconfigure.elasticsearch;

import org.springframework.context.annotation.Condition;
import org.springframework.context.annotation.ConditionContext;
import org.springframework.core.type.AnnotatedTypeMetadata;

public class IsSnifferAvailableOnClasspathCondition implements Condition {
Copy link
Member

Choose a reason for hiding this comment

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

This should go away. @ConditionalOnClass already does that.

@Override
public boolean matches(ConditionContext context, AnnotatedTypeMetadata metadata) {
return isSnifferJarOnClasspath();
}
private boolean isSnifferJarOnClasspath()
{
try {
Class.forName("org.elasticsearch.client.sniff.Sniffer");
return true;
} catch (ClassNotFoundException e) {
return false;
}
}
}
51 changes: 51 additions & 0 deletions ...ework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfigurationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@

package org.springframework.boot.autoconfigure.elasticsearch;

import java.io.IOException;
import java.time.Duration;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import org.apache.http.HttpHost;
Expand All @@ -34,6 +36,10 @@
import org.elasticsearch.client.RestClient;
import org.elasticsearch.client.RestClientBuilder;
import org.elasticsearch.client.RestHighLevelClient;
import org.elasticsearch.client.sniff.NodesSniffer;
import org.elasticsearch.client.sniff.Sniffer;
import org.elasticsearch.client.sniff.SnifferBuilder;
import org.junit.Assert;
import org.junit.jupiter.api.Test;
import org.testcontainers.elasticsearch.ElasticsearchContainer;
import org.testcontainers.junit.jupiter.Container;
Expand Down Expand Up @@ -220,6 +226,51 @@ void configureUriWithUsernameAndPasswordWhenUsernameAndPasswordPropertiesSet() {
});
}

@Test
void configureShouldOnlyCreateSnifferInstance() {
this.contextRunner.run(
(context) -> assertThat(context).doesNotHaveBean(RestClient.class)
.hasSingleBean(RestHighLevelClient.class));
this.contextRunner.run(
Copy link
Member

Choose a reason for hiding this comment

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

A single test shouldn't run the context twice ideally.

(context) -> assertThat(context).hasSingleBean(Sniffer.class));
}

@Test
void configureShouldHaveSnifferInstance() {
this.contextRunner.run(
(context) -> assertThat(context).doesNotHaveBean(RestClient.class)
.hasSingleBean(RestHighLevelClient.class));
this.contextRunner.run(
(context) -> {
assertThat(context).hasSingleBean(Sniffer.class);
Assert.assertNotNull(context.getBean(Sniffer.class));
});
}

@Test
void configureWithCustomSetIntervalProperties() {
this.contextRunner.withPropertyValues("spring.elasticsearch.rest.sniffInterval=15s, " +
Copy link
Member

Choose a reason for hiding this comment

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

15s, is not a valid duration. Did you forget to complete this test maybe?

"spring.elasticsearch.rest.sniffFailureDelay=15s").run((context) -> {
Assert.assertNotNull(context.getBean(Sniffer.class));
});
}

Copy link
Member

Choose a reason for hiding this comment

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

Two tests are missing here.

One that assert what happens when the library is not on the classpath, as I've indicated in my previous review.

One that assert that a custom Sniffer instance is used rather creating one here. This custom instance should probably have a dependency on the high level client we auto-configure to make this a bit more realistic.

@Test
void configureWithNoAutoConfigurationPropertySet() {
this.contextRunner.run((context) -> {
assertThat(context).doesNotHaveBean(Sniffer.class);
Assert.assertNull(context.getBean(Sniffer.class));
});
}

@Test
void configureWhenCustomSnifferWhenPresent() {
this.contextRunner.withBean("customSniffer", Sniffer.class, () -> mock(Sniffer.class))
.run((context) -> assertThat(context).hasSingleBean(RestHighLevelClient.class)
.hasSingleBean(Sniffer.class).hasBean("customSniffer"));
}


@Configuration(proxyBeanMethods = false)
static class BuilderCustomizerConfiguration {

Expand Down