Skip to content
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

Jump to bottom

CassandraHealthIndicator runs a query that fails on some Consistency Levels #20709

Closed
wants to merge 2 commits into from
Closed

CassandraHealthIndicator runs a query that fails on some Consistency Levels #20709

wants to merge 2 commits into from

Conversation

adutra
Copy link
Contributor

@adutra adutra commented Mar 28, 2020

The system keyspace has a replication factor of 1 and is local to each node; it is therefore recommended to query system.local with a consistency level of ONE or LOCAL_ONE.

Stronger consistency levels may result in an Unavailable error, but this does not mean that the node is down.

This patch does not include additional tests because it is hard to test the change in a stubbed environment; obviously, the existing tests still pass. Let me know if I can contribute an integration test for that.

@adutra
Use LOCAL_ONE when querying system.local - Fixes spring-projectsgh-17768
f4705a6 


The system keyspace has a replication factor of 1 and is local
to each node; it is therefore recommended to query system.local
with a consistency level of ONE or LOCAL_ONE.

Stronger consistency levels may result in an Unavailable error,
but this does not mean that the node is down.
@pivotal-issuemaster
Copy link

@adutra Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 28, 2020
@pivotal-issuemaster
Copy link

@adutra Thank you for signing the Contributor License Agreement!

@snicoll snicoll mentioned this pull request Mar 28, 2020
Closed
@snicoll snicoll added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 28, 2020
@snicoll snicoll added this to the 2.1.14 milestone Mar 28, 2020
@snicoll snicoll changed the title Use LOCAL_ONE when querying system.local - Fixes gh-17768 CassandraHealthIndicator runs a query that fails on some Consistency Levels Mar 28, 2020
@snicoll snicoll self-assigned this Mar 28, 2020
snicoll pushed a commit that referenced this pull request Mar 28, 2020
@adutra @snicoll
Use LOCAL_ONE when querying system.local
851f631 
The system keyspace has a replication factor of 1 and is local to each
node; it is therefore recommended to query system.local with a
consistency level of ONE or LOCAL_ONE.

Stronger consistency levels may result in an Unavailable error, but this
does not mean that the node is down.

See gh-20709
snicoll added a commit that referenced this pull request Mar 28, 2020
@snicoll
Polish "Use LOCAL_ONE when querying system.local"
63be167 
See gh-20709
adutra
adutra commented Mar 28, 2020
View reviewed changes
...uator/src/main/java/org/springframework/boot/actuate/cassandra/CassandraHealthIndicator.java
@@ -52,8 +56,7 @@ public CassandraHealthIndicator(CassandraOperations cassandraOperations) {

@Override
protected void doHealthCheck(Health.Builder builder) throws Exception {
SimpleStatement select = SimpleStatement.newInstance("SELECT release_version FROM system.local");
ResultSet results = this.cassandraOperations.getCqlOperations().queryForResultSet(select);
ResultSet results = this.cassandraOperations.getCqlOperations().queryForResultSet(SELECT);
if (results.isFullyFetched()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally think that this check is inaccurate here since this method will always return true for this query. As a consequence, the code beneath the if block is never reached and the health indicator never reports the release version. But since this is not related to the fix I left it for another PR.

This was referenced Mar 28, 2020
Closed
Closed
@snicoll snicoll closed this in 1f2a655 Mar 28, 2020
@snicoll
Copy link
Member

snicoll commented Mar 28, 2020

@adutra thank you very much for making your first contribution to Spring Boot.

We had to fix this one in 2.1.x and 2.2.x so I've adapted your contribution to the previous Cassandra API, see this commit for 2.1.x. Along the way, I've harmonized that check that you've just said is faulty so please let me know what it should be for the new and the previous API.

@adutra
Copy link
Contributor Author

adutra commented Mar 28, 2020

@snicoll I would simply remove the check, since the query can either fail or return one row exactly. The reactive version of the check does that btw.

Speaking of reactive, CassandraReactiveHealthIndicator should be amended as well. I just pushed another commit to my branch but too late :-) Would you mind cherry-picking adutra@2f0df4f ? Thanks!

@snicoll
Copy link
Member

snicoll commented Mar 28, 2020

I would simply remove the check, since the query can either fail or return one row exactly.

Does that apply to the previous version of the SDK as well?

@snicoll snicoll mentioned this pull request Mar 28, 2020
Closed
@adutra
Copy link
Contributor Author

adutra commented Mar 28, 2020

I would simply remove the check, since the query can either fail or return one row exactly.

Does that apply to the previous version of the SDK as well?

Yes.

snicoll added a commit that referenced this pull request Mar 29, 2020
@snicoll
Use LOCAL_ONE when querying system.local
c9e32aa 
This commit is a follow-up of gh-20709 to apply the same consistency
level to the Cassandra reactive health indicator.

Closes gh-20713
@snicoll snicoll mentioned this pull request Mar 29, 2020
Closed
@snicoll
Copy link
Member

snicoll commented Mar 29, 2020

@adutra I've harmonized the reactive health indicator. I've also created an extra bug for the fact we never expose the version attribute (see #20179). Thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@snicoll snicoll

Labels
type: bug A general bug
Projects
None yet
Milestone
2.1.14
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@adutra @pivotal-issuemaster @snicoll @spring-projects-issues