Improve CassandraHealthIndicator with more robust mechanism #23041

tomekl007 · 2020-08-21T14:59:37Z

No description provided.

adutra · 2020-08-21T16:54:45Z

To expand on what I said in #22901 : the changes proposed here are isofunctional. Users shouldn't see the slightest change in the way health reports appear in monitoring systems. However we do expect the checks to be performed almost instantaneously: these should never again risk a timeout.

adutra · 2020-08-21T16:56:44Z

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

+		// fill details with version of the first node (if the version is not null)
+		nodes.stream().map(Node::getCassandraVersion).filter(Objects::nonNull).findFirst()
+				.ifPresent((version) -> builder.withDetail("version", version));
+		return builder.build();


We now have this logic repeated 4 times. Maybe it's time to extract this to a helper class?

Good suggestion, I was thinking about the same. I didn't extract it yet because it was duplicated for the current health check logic as well. So if the spring-boot team is ok with extracting this to a separate component, I am +1 on this.

tomekl007 · 2020-09-02T07:16:43Z

When should I expect a review for this PR?

snicoll

Thanks for the PR. I've made some suggestions. I'd also like we keep the Spring Data equivalent untouched as they are deprecated anyway.

snicoll · 2020-09-08T12:43:53Z

...src/main/java/org/springframework/boot/actuate/cassandra/CassandraDriverHealthIndicator.java

+
+		// fill details with version of the first node (if the version is not null)
+		nodes.stream().map(Node::getCassandraVersion).filter(Objects::nonNull).findFirst()
+				.ifPresent((version) -> builder.withDetail("version", version));


What are the reasons for which a node wouldn't report a version? I think the first map operation should rather return the first node that's up and we should then use the information provided by that node.

What are the reasons for which a node wouldn't report a version?

A healthy Cassandra installation would always report its version. But we've seen some edge cases in the past: firstly, the system.peers table is subject to corruption (intentional or accidental); secondly, this may happen when a node is being upgraded. This is the reason why com.datastax.oss.driver.api.core.metadata.Node#getCassandraVersion is annotated with @Nullable.

Thanks for the feedback @adutra. In the case we keep the node that states UP, wouldn't that be correct to assume its version is set? Can you have a node reporting UP and not providing a version at the same time?

Can you have a node reporting UP and not providing a version at the same time?

It is technically possible, although very unlikely.

snicoll · 2020-09-08T12:45:15Z

...src/main/java/org/springframework/boot/actuate/cassandra/CassandraDriverHealthIndicator.java

-			builder.withDetail("version", row.getString(0));
+		Collection<Node> nodes = this.session.getMetadata().getNodes().values();
+		boolean atLeastOneUp = nodes.stream().map(Node::getState).anyMatch((state) -> state == NodeState.UP);
+		if (atLeastOneUp) {


This if/else branch should be simplified with a ternary operator using builder.status.

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

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

@@ -31,13 +36,11 @@
 *
 * @author Julien Dubois
 * @author Alexandre Dutra
+ * @author Tomasz Lelek


We've decided to deprecate this class. Please revert those changes.

snicoll · 2020-09-08T12:45:56Z

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

 import org.springframework.util.Assert;

 /**
 * A {@link ReactiveHealthIndicator} for Cassandra.
 *
 * @author Artsiom Yudovin
+ * @author Tomasz Lelek


We've decided to deprecate this class. Please revert those changes.

snicoll · 2020-09-08T12:47:26Z

...est/java/org/springframework/boot/actuate/cassandra/CassandraDriverHealthIndicatorTests.java

@@ -45,30 +55,86 @@ void createWhenCqlSessionIsNullShouldThrowException() {
 		assertThatIllegalArgumentException().isThrownBy(() -> new CassandraDriverHealthIndicator(null));
 	}

+	@ParameterizedTest
+	@MethodSource
+	void reportCassandraHealthCheck(Map<UUID, Node> nodes, Status expectedStatus) {


I am not a big fan of @ParameterizedTest for this and this looks inconsistent with all the other test. We have rather dedicated test where the expectations are described, potentially sharing a private method that setup the infrastructure.

Can you please give that a try? If you need inspiration, the other health indicator tests should help.

snicoll · 2020-09-08T12:47:40Z

.../org/springframework/boot/actuate/cassandra/CassandraDriverReactiveHealthIndicatorTests.java

@@ -50,29 +56,79 @@ void createWhenCqlSessionIsNullShouldThrowException() {
 		assertThatIllegalArgumentException().isThrownBy(() -> new CassandraDriverReactiveHealthIndicator(null));
 	}

+	@ParameterizedTest
+	@MethodSource
+	void reportCassandraHealthCheck(Map<UUID, Node> nodes, Status expectedStatus) {


snicoll · 2020-09-08T12:47:52Z

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


 /**
 * Tests for {@link CassandraHealthIndicator}.
 *
 * @author Oleksii Bondar
 * @author Stephane Nicoll
+ * @author Tomasz Lelek


This should be reverted

snicoll · 2020-09-08T12:48:03Z

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


 /**
 * Tests for {@link CassandraReactiveHealthIndicator}.
 *
 * @author Artsiom Yudovin
+ * @author Tomasz Lelek


This should be reverted.

snicoll · 2020-09-23T12:40:06Z

@tomekl007 do you have time to process feedback on this PR?

tomekl007 · 2020-09-23T13:21:57Z

@tomekl007 do you have time to process feedback on this PR?

Hello, unfortunately currently I don't have time to adapt it. Someone from the team should be able to take a look at it in the next week (that starts 28.09)

adutra · 2020-10-01T09:24:48Z

@snicoll sorry for the delay, our team had to shift to a few urgent projects as Cassandra 4.0 GA approaches.

I addressed all your comments and requests, let me know what you think.

adutra · 2020-10-01T09:25:46Z

...src/main/java/org/springframework/boot/actuate/cassandra/CassandraDriverHealthIndicator.java

-		if (row != null && !row.isNull(0)) {
-			builder.withDetail("version", row.getString(0));
-		}
+		Collection<Node> nodes = this.session.getMetadata().getNodes().values();


This logic is still duplicated twice. Do you think it's worth extracting it to a separate, shared component?

snicoll · 2020-10-01T15:58:03Z

@snicoll sorry for the delay, our team had to shift to a few urgent projects as Cassandra 4.0 GA approaches.

No worries, we still have time. Don't worry about reverting code, I can easily do that as part of merging this issue.

See gh-23041

snicoll · 2020-10-02T14:13:40Z

@tomekl007 thank you for making your first contribution to Spring Boot. @adutra thank you for following up!

Improve CassandraHealthIndicator with more robust mechanism fixes spr…

a678b45

…ing-projectsgh-22901

tomekl007 mentioned this pull request Aug 21, 2020

Improve CassandraHealthIndicator with more robust mechanism #22901

Closed

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

adutra reviewed Aug 21, 2020

View reviewed changes

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

snicoll added this to the 2.4.x milestone Sep 8, 2020

snicoll self-assigned this Sep 8, 2020

snicoll requested changes Sep 8, 2020

View reviewed changes

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

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 Sep 23, 2020

snicoll added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Sep 24, 2020

adutra added 4 commits October 1, 2020 10:29

Use ternary operator to set the builder status

ab809e3

Revert changes to deprecated classes

ca04c9e

Replace parameterized test with simple tests

09eaca8

Simplify logic to set the Cassandra version

0f9ee8d

adutra reviewed Oct 1, 2020

View reviewed changes

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 Oct 1, 2020

adutra added 2 commits October 1, 2020 17:49

Revert changes to deprecated classes (2nd attempt)

6ab3e81

Revert changes to deprecated test classes (2nd attempt)

ccb7d89

snicoll changed the title ~~Improve CassandraHealthIndicator with more robust mechanism fixes gh-22901~~ Improve CassandraHealthIndicator with more robust mechanism Oct 2, 2020

snicoll modified the milestones: 2.4.x, 2.4.0-M4 Oct 2, 2020

snicoll pushed a commit that referenced this pull request Oct 2, 2020

Improve Cassandra health indicator with more robust mechanism

8e5a041

See gh-23041

snicoll added a commit that referenced this pull request Oct 2, 2020

Polish "Improve Cassandra health indicator with more robust mechanism"

6f08e97

See gh-23041

snicoll closed this in 48c2f4d Oct 2, 2020

Improve CassandraHealthIndicator with more robust mechanism #23041

Improve CassandraHealthIndicator with more robust mechanism #23041

Uh oh!

Conversation

tomekl007 commented Aug 21, 2020

Uh oh!

adutra commented Aug 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomekl007 commented Sep 2, 2020

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!

snicoll Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Sep 23, 2020

Uh oh!

tomekl007 commented Sep 23, 2020

Uh oh!

adutra commented Oct 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snicoll commented Oct 1, 2020

Uh oh!

snicoll commented Oct 2, 2020

Uh oh!

Uh oh!

snicoll Sep 11, 2020 •

edited

Loading