Add support for Elasticsearch Sniffer #24340

somayaj · 2020-12-05T17:07:12Z

Added autoconfig support and tests for the elastic search sniffer.

snicoll

Thanks for the PR, they are a number of things to reconsider. Can you please have a look?

spring-boot-project/spring-boot-autoconfigure/build.gradle

snicoll · 2020-12-21T15:34:34Z

...gframework/boot/autoconfigure/elasticsearch/sniff/ElasticsearchSnifferAutoConfiguration.java

+@ConditionalOnClass(RestHighLevelClient.class)
+@ConditionalOnMissingBean(RestClient.class)
+@EnableConfigurationProperties(ElasticsearchSnifferProperties.class)
+public class ElasticsearchSnifferAutoConfiguration {


I don't think it should be a separate auto-configuration. Rather it should be part of what we already auto-configure.

snicoll · 2020-12-21T15:35:50Z

...gframework/boot/autoconfigure/elasticsearch/sniff/ElasticsearchSnifferAutoConfiguration.java

+ */
+@Configuration(proxyBeanMethods = false)
+@ConditionalOnClass(RestHighLevelClient.class)
+@ConditionalOnMissingBean(RestClient.class)


The purpose of the sniffer is to work based on a RestClient but this auto-configuration doesn't do anything if such bean is present?

snicoll · 2020-12-21T15:36:30Z

...gframework/boot/autoconfigure/elasticsearch/sniff/ElasticsearchSnifferAutoConfiguration.java

+	static class SnifferBuilderConfiguration {
+
+		@Bean
+		SnifferBuilderCustomizer defaultSnifferBuilderCustomizer(ElasticsearchSnifferProperties properties) {


For simple use case like this one there is no need to create a customizer for what the auto-configuration does. We can smart small and configure the sniffer based on the properties only.

snicoll · 2020-12-21T15:39:16Z

...gframework/boot/autoconfigure/elasticsearch/sniff/ElasticsearchSnifferAutoConfiguration.java

+		@Bean
+		Sniffer elasticsearchSnifferBuilder(ElasticsearchSnifferProperties properties,
+											RestClient elasticsearchRestClient) {
+			return Sniffer.builder(elasticsearchRestClient).build();


Spring Boot no longer exposes a RestClient. Rather, it is created internally as part of ElasticsearchRestClientAutoConfiguration which is where this code should go.

The documentation also states that order in which those are destroy is important:

It is important to close the Sniffer so that its background thread gets properly shutdown and all of its resources are released. The Sniffer object should have the same lifecycle as the RestClient and get closed right before the client:

snicoll · 2020-12-21T15:40:14Z

...g/springframework/boot/autoconfigure/elasticsearch/sniff/ElasticsearchSnifferProperties.java

+	/**
+	 * Sniffer interval millis
+	 */
+	private Duration sniffIntervalMillis = Duration.ofMillis(1L);


A Duration property should not have mills (or any unit) in its name since users are free to chose a different unit if they want to.

Also, the default value is wrong, it should be 5 min.

snicoll · 2020-12-21T15:42:02Z

...g/springframework/boot/autoconfigure/elasticsearch/sniff/ElasticsearchSnifferProperties.java

+	/**
+	 * Sniffer failure delay millis.
+	 */
+	private Duration sniffFailureDelayMillis = Duration.ofMillis(1L);


The default value is not 1ms but 1 minute.

snicoll · 2020-12-21T15:42:49Z

...ava/org/springframework/boot/autoconfigure/elasticsearch/sniff/SnifferBuilderCustomizer.java

+ *
+ */
+@FunctionalInterface
+public interface SnifferBuilderCustomizer {


We don't need this for a first pass. Those who want to configure the NodesSniffer can do so with their own Sniffer instance.

snicoll · 2020-12-21T15:43:26Z

...ework/boot/autoconfigure/elasticsearch/sniff/ElasticsearchSnifferAutoConfigurationTests.java

+ * @author Asha Somayajula
+ */
+@Testcontainers(disabledWithoutDocker = true)
+class ElasticsearchSnifferAutoConfigurationTests {


This should move to the main ElasticsearchRestClientAutoConfigurationTests. A test should be added with a filtered class loader on the sniffer library to validate things are working as expected.

snicoll

Thanks for the follow-up but some parts of my previous review were not processed. Can you please have a look again?

snicoll · 2020-12-22T08:58:01Z

...ringframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfiguration.java

+											RestClient elasticSearchRestClient) {
+			return Sniffer.builder(elasticSearchRestClient)
+					.setSniffIntervalMillis(
+							Math.toIntExact(properties.getSniffInterval().getSeconds() * 1000))


This isn't necessary. The duration gives you a way to get the ms directly.

snicoll · 2020-12-22T08:58:42Z

...ringframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfiguration.java

@@ -80,6 +83,17 @@ RestClientBuilder elasticsearchRestClientBuilder(ElasticsearchRestClientProperti
 			return builder;
 		}

+		@Bean
+		Sniffer elasticsearchSnifferBuilder(ElasticsearchSnifferProperties properties,


This doesn't check that the sniffer library is on the classpath. If it isn't, this bean should not be defined at all. It's also missing a @ConditionalOnMissingBean in case the user has created their own.

snicoll · 2020-12-22T08:59:39Z

...ava/org/springframework/boot/autoconfigure/elasticsearch/ElasticsearchSnifferProperties.java

+ *
+ */
+@ConfigurationProperties(prefix = "spring.elasticsearch.sniff")
+public class ElasticsearchSnifferProperties {


This should move to ElasticsearchRestClientProperties. There is no reason for those two attributes to be in their own type since they relate to Elasticsearch's rest support anyway.

snicoll · 2020-12-22T09:01:15Z

...ramework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfigurationTests.java

+			Assert.assertNotNull(sniffer);
+			});
+	}
+


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.

snicoll

Thanks for the update again but the tests you've added don't pass. Can you please build the module and make sure the tests are working before submitting further changes?

If you want me to take over, I can also do that.

snicoll · 2020-12-23T08:52:57Z

...ringframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfiguration.java

+		@Conditional(IsSnifferAvailableOnClasspathCondition.class)
+		@ConditionalOnMissingBean(RestHighLevelClient.class)
+		Sniffer elasticsearchSnifferBuilder(ElasticsearchRestClientProperties properties,
+											RestClient elasticSearchRestClient) {


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.

snicoll · 2020-12-23T08:53:14Z

...ringframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfiguration.java

+		@Bean
+		@Conditional(IsSnifferAvailableOnClasspathCondition.class)
+		@ConditionalOnMissingBean(RestHighLevelClient.class)
+		Sniffer elasticsearchSnifferBuilder(ElasticsearchRestClientProperties properties,


The builder in the name sounds unnecessary.

snicoll · 2020-12-23T08:53:53Z

...ringframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfiguration.java

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

+		@Bean
+		@Conditional(IsSnifferAvailableOnClasspathCondition.class)


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).

snicoll · 2020-12-23T08:54:44Z

.../org/springframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientProperties.java

+	/**
+	 * Sniffer interval
+	 */
+	private Duration sniffInterval = Duration.ofMinutes(5L);


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

snicoll · 2020-12-23T08:55:08Z

.../org/springframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientProperties.java

@@ -56,6 +56,14 @@
 	 * Read timeout.
 	 */
 	private Duration readTimeout = Duration.ofSeconds(30);
+	/**
+	 * Sniffer interval


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

snicoll · 2020-12-23T08:55:32Z

.../org/springframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientProperties.java

@@ -97,4 +105,19 @@ public void setReadTimeout(Duration readTimeout) {
 		this.readTimeout = readTimeout;
 	}

+	public Duration getSniffInterval() {
+		return sniffInterval;


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

snicoll · 2020-12-23T08:55:38Z

.../org/springframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientProperties.java

+	}
+
+	public Duration getSniffFailureDelay() {
+		return sniffFailureDelay;


snicoll · 2020-12-23T08:55:53Z

...springframework/boot/autoconfigure/elasticsearch/IsSnifferAvailableOnClasspathCondition.java

+import org.springframework.context.annotation.ConditionContext;
+import org.springframework.core.type.AnnotatedTypeMetadata;
+
+public class IsSnifferAvailableOnClasspathCondition implements Condition {


This should go away. @ConditionalOnClass already does that.

snicoll · 2020-12-23T08:56:11Z

...ramework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfigurationTests.java

+		this.contextRunner.run(
+				(context) -> assertThat(context).doesNotHaveBean(RestClient.class)
+				.hasSingleBean(RestHighLevelClient.class));
+		this.contextRunner.run(


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

snicoll · 2020-12-23T08:56:39Z

...ramework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfigurationTests.java

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


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

somayaj · 2020-12-23T14:39:17Z

Thanks for the feedback. I believe all of the changes requested are in place now.

snicoll · 2020-12-23T16:15:37Z

I believe all of the changes requested are in place now.

Unfortunately, 4 of the 5 tests you've added are still failing for me. It might be because you don't have docker running locally and the tests are skipped for you (see @Testcontainers(disabledWithoutDocker = true) at the top of the test class).

Thank you very much for the follow-up and the feedback on the reviews. Unfortunately, there are quite a lot of pieces that still need more work and I've decided to take over. Please review 1fd17cf for more details. Thanks for the PR, in any case!

somayaj · 2020-12-23T17:00:29Z

Thanks for considering. Regards, Asha

…

On Wed, Dec 23, 2020 at 10:15 AM Stéphane Nicoll ***@***.***> wrote: I believe all of the changes requested are in place now. Unfortunately, 4 of the 5 tests you've added are still failing for me. It might be because you don't have docker running locally and the tests are skipped for you (see @testcontainers(disabledWithoutDocker = true) at the top of the test class). Thank you very much for the follow-up and the feedback on the reviews. Unfortunately, there are quite a lot of pieces that still need more work and I've decided to take over. Please review 1fd17cf <1fd17cf> for more details. Thanks for the PR, in any case! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#24340 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIRAZGKMLHLFB4Q4JHAUJW3SWIJTTANCNFSM4UOVTK2Q> .

See spring-projectsgh-24340

Closes spring-projectsgh-24340

Provide out-of-the-box support for Elasticsearch Sniffer #24174

b7fd39a

Added autoconfig support and tests for the elastic search sniffer.

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 5, 2020

scottfrederick added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 9, 2020

scottfrederick added this to the 2.5.x milestone Dec 9, 2020

scottfrederick changed the title ~~Provide out-of-the-box support for Elasticsearch Sniffer #24174~~ Provide out-of-the-box support for Elasticsearch Sniffer Dec 9, 2020

scottfrederick mentioned this pull request Dec 9, 2020

Provide out-of-the-box support for Elasticsearch Sniffer #24174

Closed

snicoll changed the title ~~Provide out-of-the-box support for Elasticsearch Sniffer~~ Add support for Elasticsearch Sniffer Dec 21, 2020

snicoll requested changes Dec 21, 2020

View reviewed changes

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Dec 21, 2020

Fixes per review.

5abf542

snicoll requested changes Dec 22, 2020

View reviewed changes

Fixes per review.

9ca5436

snicoll requested changes Dec 23, 2020

View reviewed changes

Fixes per review.

c7932c3

spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 23, 2020

snicoll closed this Dec 23, 2020

snicoll removed this from the 2.5.x milestone Dec 23, 2020

snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed status: feedback-provided Feedback has been provided type: enhancement A general enhancement labels Dec 23, 2020

humaolin pushed a commit to humaolin/spring-boot that referenced this pull request May 7, 2022

Document Objenesis license in license.txt

58d1198

See spring-projectsgh-24340

humaolin pushed a commit to humaolin/spring-boot that referenced this pull request May 7, 2022

Document Objenesis license in license.txt

1cfa954

Closes spring-projectsgh-24340

humaolin pushed a commit to humaolin/spring-boot that referenced this pull request May 7, 2022

Document Objenesis license in license.txt

c91b47f

Closes spring-projectsgh-24340

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Elasticsearch Sniffer #24340

Add support for Elasticsearch Sniffer #24340

somayaj commented Dec 5, 2020

snicoll left a comment

snicoll Dec 21, 2020

snicoll Dec 21, 2020

snicoll Dec 21, 2020

snicoll Dec 21, 2020

snicoll Dec 21, 2020

snicoll Dec 21, 2020

snicoll Dec 21, 2020

snicoll Dec 21, 2020

snicoll left a comment

snicoll Dec 22, 2020

snicoll Dec 22, 2020

snicoll Dec 22, 2020 •

edited

Loading

snicoll Dec 22, 2020

snicoll left a comment

snicoll Dec 23, 2020

snicoll Dec 23, 2020

snicoll Dec 23, 2020

snicoll Dec 23, 2020

snicoll Dec 23, 2020

snicoll Dec 23, 2020

snicoll Dec 23, 2020

snicoll Dec 23, 2020

snicoll Dec 23, 2020

snicoll Dec 23, 2020

somayaj commented Dec 23, 2020

snicoll commented Dec 23, 2020

somayaj commented Dec 23, 2020 via email

Add support for Elasticsearch Sniffer #24340

Add support for Elasticsearch Sniffer #24340

Conversation

somayaj commented Dec 5, 2020

snicoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snicoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snicoll Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snicoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somayaj commented Dec 23, 2020

snicoll commented Dec 23, 2020

somayaj commented Dec 23, 2020 via email

snicoll Dec 22, 2020 •

edited

Loading