Add out-of-the-box health indicator for reactive MongoDB #11997

qinnnyul · 2018-02-12T07:02:52Z

Add out-of-the-box health indicator for reactive MongoDB and write few test cases to cover new changes. #11766

snicoll

Thanks for the PR. I've made a few suggestions.

snicoll · 2018-02-12T07:27:56Z

...va/org/springframework/boot/autoconfigure/data/mongo/MongoReactiveDataAutoConfiguration.java


+	@Bean
+	@ConditionalOnMissingBean(MongoConverter.class)
+	public MappingMongoConverter mappingMongoConverter() {


How is this related to this change?

It is required by “ReactiveMongoTemplate” bean while it is missing from Spring IOC factory, that is why I provided default implementation of MongoConverter here when it is missing in context, otherwise, it will complains “unsatisfied dependencies ”

@qinnnyul sorry I am not following. Why is it required?

@snicoll In current class, we can see “reactiveMongoTemplate” method, when it constructs instance, it needs spring to inject two type of beans(one is ReactiveMongoDatabaseFactory,another is MongoConverter), however, it only has method to construct ReactiveMongoDatabaseFactory before, another is missing. You can find the similar way in MongoDataAutoConfiguration.class, but I did not want to mix it up, that is why I added default implementation of MongoConverter here, does that make sense to you? what do you recommend?

I saw that but this is provided by MongoDataAutoConfiguration and there isn't a reason to copy/paste that here. Can you please rework the PR so that tests also include MongoDataAutoConfiguration?

See also #12001

@snicoll thanks for your suggestion, but I am not sure if it is good idea to make an explicit link between MongoReactiveDataAutoConfiguration and MongoDataAutoConfiguration, cause it has different conditions. Besides that, even I tried what you suggested, it throws out the following error: “Error creating bean with name 'reactiveMongoTemplate' defined in class path resource [org/springframework/boot/autoconfigure/data/mongo/MongoReactiveDataAutoConfiguration.class]: Unsatisfied dependency expressed through method 'reactiveMongoTemplate' parameter 1; nested exception is org.springframework.beans.factory.NoSuchBeanDefinitionException: No qualifying bean of type 'org.springframework.data.mongodb.core.convert.MongoConverter' available: “

snicoll · 2018-02-12T07:28:29Z

...r/src/test/java/org/springframework/boot/actuate/mongo/MongoReactiveHealthIndicatorTest.java

+ */
+
+public class MongoReactiveHealthIndicatorTest {
+	@Mock


We don't use this construct, please mock the template in the test as you've done for the rest.

snicoll · 2018-02-12T07:29:21Z

spring-boot-project/spring-boot-actuator-autoconfigure/pom.xml

 			<artifactId>spring-data-mongodb</artifactId>
 			<optional>true</optional>
 		</dependency>
+		<dependency>


Dependencies are ordered alphabetically. Please reorder those two entries.

dependencies been re-ordered.

…es order.

…class

snicoll · 2018-02-12T14:28:27Z

...va/org/springframework/boot/autoconfigure/data/mongo/MongoReactiveDataAutoConfiguration.java

 @ConditionalOnClass({ MongoClient.class, ReactiveMongoTemplate.class })
 @EnableConfigurationProperties(MongoProperties.class)
-@AutoConfigureAfter(MongoReactiveAutoConfiguration.class)
+@AutoConfigureAfter({ MongoReactiveAutoConfiguration.class, MongoDataAutoConfiguration.class })


Please rebase your PR rather than changing something that has already been changed.

snicoll · 2018-02-12T14:28:55Z

...r/src/test/java/org/springframework/boot/actuate/mongo/MongoReactiveHealthIndicatorTest.java

+ */
+
+public class MongoReactiveHealthIndicatorTest {
+	private MongoReactiveHealthIndicator mongoReactiveHealthIndicator;


This doesn't need to be a field

snicoll · 2018-02-12T14:29:09Z

...r/src/test/java/org/springframework/boot/actuate/mongo/MongoReactiveHealthIndicatorTest.java

+ *
+ * @author Yulin Qin
+ */
+


Unnecessary extra space.

snicoll · 2018-02-12T14:29:55Z

...ramework/boot/actuate/autoconfigure/mongo/MongoReactiveHealthIndicatorAutoConfiguration.java

+@ConditionalOnBean(ReactiveMongoTemplate.class)
+@ConditionalOnEnabledHealthIndicator("mongo")
+@AutoConfigureBefore(HealthIndicatorAutoConfiguration.class)
+@AutoConfigureAfter({MongoReactiveAutoConfiguration.class, MongoReactiveDataAutoConfiguration.class})


I think only the latter is necessary here.

snicoll · 2018-02-12T14:30:22Z

spring-boot-project/spring-boot-actuator-autoconfigure/pom.xml

 			<optional>true</optional>
 		</dependency>
+		<dependency>
+			<groupId>org.mongodb</groupId>


This isn't sorted properly still (m is before s)

See gh-11997

…tive-mongo * pr/11997: Polish "Add health indicator for reactive MongoDB" Add health indicator for reactive MongoDB

snicoll · 2018-02-13T10:34:25Z

@qinnnyul Thanks for working on my comments. Unfortunately, the state of this PR is still poor so I decided to polish it myself rather than asking you to review again. Please review the following and take it into account for any future PR:

The PR wasn't properly rebased on master. You can run git rebase origin/master on your branch once you've pulled the latest change (git fetch origin). Some conflicts needed to be fixed. Once that's done, please squash your commit into one and push force on your branch
The build did not pass. Did you run it? Some tests were failing because the reactive driver wasn't available. Once I added it, other tests start failing as they didn't expect reactive mongo to be there
The reactive health indicator wasn't shadowing the "regular" health indicator so an app with the reactive mongo driver had two health indicators for MongoDB. You wrote a test that was supposed to exercise that but it wasn't importing the regular mongo auto-configuration. Once I added that, it failed
reactive health indicators are listed in the doc

qinnnyul · 2018-02-13T11:57:29Z

@snicoll thanks, will keep it in mind.

Add Auto-config health indicator for reactive MongoDB

0cd1467

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 12, 2018

snicoll requested changes Feb 12, 2018

View reviewed changes

refactor codes based on spring-boot test style, and adjust dependenci…

dff7a49

…es order.

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Feb 12, 2018

reuse existing MongoConverter provided by MongoDataAutoConfiguration.…

341b400

…class

snicoll requested changes Feb 12, 2018

View reviewed changes

qinnnyul added 3 commits February 12, 2018 22:40

revert it first, will rebase to get latest change from master

35a7764

Merge branch 'master' into add-health-indicator-for-reactive-mongo

47a74a9

make changes according to code review comments.

b55ef3c

snicoll self-assigned this Feb 13, 2018

snicoll added type: enhancement A general enhancement priority: normal and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Feb 13, 2018

snicoll added this to the 2.0.0.RC2 milestone Feb 13, 2018

snicoll pushed a commit that referenced this pull request Feb 13, 2018

Add health indicator for reactive MongoDB

eee2694

See gh-11997

snicoll closed this in 28f5392 Feb 13, 2018

snicoll added a commit that referenced this pull request Feb 13, 2018

Merge pull request #11997 from qinnnyul:add-health-indicator-for-reac…

718bc72

…tive-mongo * pr/11997: Polish "Add health indicator for reactive MongoDB" Add health indicator for reactive MongoDB

qinnnyul deleted the add-health-indicator-for-reactive-mongo branch February 13, 2018 11:32

qinnnyul restored the add-health-indicator-for-reactive-mongo branch February 13, 2018 12:03

qinnnyul deleted the add-health-indicator-for-reactive-mongo branch February 13, 2018 12:03

izeye mentioned this pull request Jul 31, 2018

Add out-of-the-box health indicator for reactive MongoDB #11766

Closed

Add out-of-the-box health indicator for reactive MongoDB #11997

Add out-of-the-box health indicator for reactive MongoDB #11997

Uh oh!

Conversation

qinnnyul commented Feb 12, 2018

Uh oh!

snicoll left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snicoll commented Feb 13, 2018

Uh oh!

qinnnyul commented Feb 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants