Add Cassandra health indicator that uses CqlSession #20887

adutra · 2020-04-08T19:30:20Z

Motivation:

It is possible to use CassandraAutoConfiguration to create CqlSession
beans in an application with just a dependency on the DataStax Java driver,
without the requirement to also depend on Spring Data Cassandra.

However, it is currently not possible to do so if the application also
needs to create Actuator components. Indeed, both CassandraHealthIndicator
and CassandraReactiveHealthContributorAutoConfiguration have a hard
dependency on Spring Data Cassandra and its CassandraOperations bean.

Modifications:

This commit removes the hard dependency on Spring Data Cassandra from
CassandraHealthIndicator and CassandraReactiveHealthContributorAutoConfiguration,
and replaces all usages of CassandraOperations with direct access to the
underlying CqlSession bean.

Result:

Users are now able to create Actuator health components in their applications,
even if they use just Spring Boot and the DataStax Java driver, without
Spring Data Cassandra.

adutra · 2020-04-08T19:35:31Z

...uator/src/main/java/org/springframework/boot/actuate/cassandra/CassandraHealthIndicator.java

+		Row row = this.session.execute(SELECT).one();
+		Assert.notNull(row, "system.local should always return one row");
+		String version = row.getString(0);
+		Assert.notNull(version, "release_version should never be null");


These assertions really will never fail as any C* node would report exactly one row with one non-null column for such a CQL query. But they make static analysis tools happy.

There's no need to do that, you could check if the row is non null (since the API forces you to do that check), something like:

Row row = this.session.execute(SELECT).one(); builder.up(); if (row != null) { builder.withDetail("version", row.getString(0)); }

Isn't that slightly different? In the very unlikely case where the row would be null, in my version the health indicator would be set to DOWN, whereas in your case, it would be UP. But I will do as you suggest, since a null row is totally unrealistic anyways.

Yes, you are right, I overlooked that. Although the "These assertions really will never fail" makes me think it won't make a difference.

adutra · 2020-04-08T19:36:05Z

...c/main/java/org/springframework/boot/actuate/cassandra/CassandraReactiveHealthIndicator.java

-		return this.reactiveCassandraOperations.getReactiveCqlOperations().queryForObject(SELECT, String.class)
-				.map((version) -> builder.up().withDetail("version", version).build()).single();
+		return Mono.from(this.session.executeReactive(SELECT))
+				.map((row) -> Objects.requireNonNull(row.getString(0), "release_version should never be null"))


Same here, Objects.requireNonNull is just making my IDE happy.

IntelliJ IDEA? I think that's a false positive.

return Mono.from(this.session.executeReactive(SELECT)) .map((row) -> builder.up().withDetail("version", row.getString(0)).build());

this is more concise and doesn't yield any warning for me.

Actually GettableByIndex#getString(int) is marked @Nullable in the driver API, so it may trigger a warning depending on how you configure things. But again I don't mind really, I'll do as you suggest.

I think that's going a tad too far. The row will never be null given the contract of Publisher and onNext.

adutra · 2020-04-08T19:46:52Z

...uator/src/main/java/org/springframework/boot/actuate/cassandra/CassandraHealthIndicator.java

@@ -38,25 +39,24 @@
 	private static final SimpleStatement SELECT = SimpleStatement
 			.newInstance("SELECT release_version FROM system.local").setConsistencyLevel(ConsistencyLevel.LOCAL_ONE);

-	private CassandraOperations cassandraOperations;
-
-	public CassandraHealthIndicator() {


Removed because it doesn't buy us anything, but this is a breaking change. Let me know if you prefer to keep it.

snicoll

Thanks for the PR. I've replied to your 2 comments.

Mocking Publisher is going a step too far I think and I am not sure I want to start a Cassandra instance for such a test. I don't know if there is an alternative.

snicoll · 2020-04-09T13:47:32Z

.../src/test/java/org/springframework/boot/actuate/cassandra/CassandraHealthIndicatorTests.java

-		CassandraHealthIndicator healthIndicator = new CassandraHealthIndicator(cassandraOperations);
+		CqlSession session = mock(CqlSession.class);
+		given(session.execute(any(SimpleStatement.class)))
+				.willThrow(new DriverTimeoutException("Connection timed out (not really)"));


Please change that to Test Exception if you want to make it clear it's a test exception.

snicoll · 2020-04-09T13:48:27Z

...t/java/org/springframework/boot/actuate/cassandra/CassandraReactiveHealthIndicatorTests.java

 		}).verifyComplete();
 	}

+	private Answer<Void> mockReactiveResultSetBehavior(ReactiveRow row) {


Meh. I am not keen to start mocking a Publisher here. The fact that the return type is a Publisher + something else makes it very hard to use existing reactor types. WIthout that I could have replaced that with Mono.just.

Yeah, I don't see how to simplify this either. I'll take a note to provide a dummy implementation of ReactiveResultSet for test use cases like this one, in a future driver release.

snicoll · 2020-04-09T13:49:13Z

...mework/boot/autoconfigure/data/cassandra/CassandraReactiveRepositoriesAutoConfiguration.java

@@ -39,7 +41,7 @@
 * @see EnableReactiveCassandraRepositories
 */
 @Configuration(proxyBeanMethods = false)
-@ConditionalOnClass({ ReactiveSession.class, ReactiveCassandraRepository.class })
+@ConditionalOnClass({ ReactiveSession.class, ReactiveCassandraRepository.class, Flux.class })


I am not against that change but it has nothing to do with the task at hand. Please revert that and submit it in a separate change if you think this is worth exploring.

True, this is a mistake, I will remove.

snicoll · 2020-05-12T11:14:42Z

We don't want to break backward compatible for this one. This should be possible by keeping the existing health indicator, adding this one and updating the auto-configuration to try this one and then the existing one if none apply. Unfortunately, the existing health indicator is in the cassandra package so we can't really have an additional one without having to use a name we wouldn't like. We've discovered brainstorming on this one that #11574 could be extended to package space as well as the actuator has no notion of data vs. raw driver.

It is unfortunately too late to make a breaking change in 2.3.x so I've scheduled this for the next feature release.

adutra · 2020-05-23T15:09:57Z

I understand your concerns. I will rework this PR in the way you suggested, and should have something ready shortly. If you don't mind, since we are back to square zero anyway, I will rebase on master and squash the commits.

adutra · 2020-05-23T15:32:39Z

What do you suggest for class names?

I'm currently planning on declaring all health indicators under org.springframework.boot.actuate.cassandra and naming the new ones:

CassandraDriverHealthIndicator
CassandraDriverReactiveHealthIndicator

Is that ok?

Motivation: It is possible to use CassandraAutoConfiguration to create CqlSession beans in an application with just a dependency on the DataStax Java driver, without the requirement to also depend on Spring Data Cassandra. However, it is currently not possible to do so if the application also needs to create Actuator components. Indeed, both CassandraHealthIndicator and CassandraReactiveHealthIndicator have a hard dependency on Spring Data Cassandra and its CassandraOperations bean. Modifications: This commit creates two new health indicators that do not have any dependency on Spring Data Cassandra: CassandraDriverHealthIndicator and CassandraDriverReactiveHealthIndicator; these indicators are automatically used in lieu of CassandraHealthIndicator and CassandraReactiveHealthIndicator when Spring Data Cassandra is not available. Result: Users are now able to create Actuator health components in their applications, even if they use just Spring Boot and the DataStax Java driver, without Spring Data Cassandra.

philwebb · 2020-05-26T23:50:16Z

@adutra Those names sound sensible to me. We can always refine them if we think of something better when we merge.

This commit provides a CassandraDriverHealthIndicator and CassandraDriverReactiveHealthIndicator that do not require Spring Data. As a result, a health indicator for Cassandra is provided even if the application does not use Spring Data. See gh-20887

See gh-20887

snicoll · 2020-06-15T09:14:31Z

Thanks @adutra. I've merged your contribution with a polish commit.

snicoll · 2020-09-08T12:42:13Z

For the record, we've decided to deprecate the health indicators that rely on Spring Data (see #23226).

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

adutra commented Apr 8, 2020

View reviewed changes

adutra force-pushed the nosdc branch from 05cdc70 to 8d44702 Compare April 8, 2020 19:44

adutra commented Apr 8, 2020

View reviewed changes

adutra force-pushed the nosdc branch from 8d44702 to 62126a4 Compare April 8, 2020 19:48

snicoll changed the title ~~Remove dependency on Spring Data Cassandra from Actuator components~~ Lower required dependency for Cassandra health indicator Apr 9, 2020

snicoll requested changes Apr 9, 2020

View reviewed changes

snicoll added status: waiting-for-feedback We need additional information before we can continue for: team-attention An issue we'd like other members of the team to review labels Apr 9, 2020

philwebb self-assigned this Apr 9, 2020

philwebb removed the for: team-attention An issue we'd like other members of the team to review label Apr 9, 2020

snicoll added for: team-attention An issue we'd like other members of the team to review and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 20, 2020

philwebb assigned snicoll May 6, 2020

philwebb removed the for: team-attention An issue we'd like other members of the team to review label May 6, 2020

snicoll changed the title ~~Lower required dependency for Cassandra health indicator~~ Switch Cassandra health indicator to use CqlSession May 12, 2020

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

snicoll added this to the 2.4.x milestone May 12, 2020

adutra force-pushed the nosdc branch from eba254a to 6c10d62 Compare May 24, 2020 12:10

adutra force-pushed the nosdc branch from 6c10d62 to 7255c1c Compare May 24, 2020 12:12

adutra requested a review from snicoll May 24, 2020 13:11

snicoll changed the title ~~Switch Cassandra health indicator to use CqlSession~~ Add Cassandra health indicator that uses CqlSession Jun 15, 2020

snicoll removed this from the 2.4.x milestone Jun 15, 2020

snicoll added this to the 2.4.0-M1 milestone Jun 15, 2020

snicoll added a commit that referenced this pull request Jun 15, 2020

Polish "Add health indicator for Cassandra that uses the CqlSession"

35e069e

See gh-20887

snicoll closed this in 776cd08 Jun 15, 2020

adutra deleted the nosdc branch June 15, 2020 16:51

snicoll mentioned this pull request Sep 8, 2020

Deprecate Cassandra health indicator that uses Spring Data's CassandraOperations #23226

Closed

Add Cassandra health indicator that uses CqlSession #20887

Add Cassandra health indicator that uses CqlSession #20887

Uh oh!

Conversation

adutra commented Apr 8, 2020

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 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!

snicoll commented May 12, 2020

Uh oh!

adutra commented May 23, 2020

Uh oh!

adutra commented May 23, 2020

Uh oh!

philwebb commented May 26, 2020

Uh oh!

snicoll commented Jun 15, 2020

Uh oh!

snicoll commented Sep 8, 2020

Uh oh!

Uh oh!