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.

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

First part of #1644: C*3 from 3.11.11->3.11.12 #1646

Merged
merged 7 commits into from
Feb 23, 2022

Conversation

tatu-at-datastax
Copy link
Collaborator

@tatu-at-datastax tatu-at-datastax commented Feb 16, 2022

What this PR does:

Starting work on #1644: first upgrade Cassandra-3: separate upgrades to isolate problems.

Instructions to follow:

Which issue(s) this PR fixes:

Will eventually resolve #1644 but not with just this PR.

Checklist

  • Changes manually tested
  • Automated Tests added/updated -- relies on test coverage
  • Documentation added/updated
  • CLA Signed: DataStax CLA

@tatu-at-datastax
Copy link
Collaborator Author

Alas, immediate (unit) test failures for seemingly trivial update:

Step #4 - "cassandra-3.11": [INFO] Running io.stargate.db.cassandra.Cassandra311PersistenceActivatorTest
Step #4 - "cassandra-3.11": [INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.48 s - in io.stargate.db.cassandra.Cassandra311PersistenceActivatorTest
Step #4 - "cassandra-3.11": [INFO] 
Step #4 - "cassandra-3.11": [INFO] Results:
Step #4 - "cassandra-3.11": [INFO] 
Step #4 - "cassandra-3.11": [ERROR] Errors: 
Step #4 - "cassandra-3.11": [ERROR]   StargateQueryHandlerTest.authorizeByTokenBatchStatement:936 ? IllegalState Can...
Step #4 - "cassandra-3.11": [ERROR]   IdempotencyAnalyzerTest.shouldReturnIdempotentIfAllStatementsWithinABatchAreIdempotent:102 ? IllegalState
Step #4 - "cassandra-3.11": [ERROR]   IdempotencyAnalyzerTest.shouldReturnNonIdempotentIfAllStatementsWithinABatchAreNonIdempotent:117 ? IllegalState
Step #4 - "cassandra-3.11": [INFO] 

@tatu-at-datastax tatu-at-datastax added the dependencies Pull requests that update a dependency file label Feb 17, 2022
Copy link
Contributor

@ivansenic ivansenic left a comment

Choose a reason for hiding this comment

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

Hey @tatu-at-datastax, can you please go through the list that I made with the last update -> https://github.com/stargate/stargate/tree/master/persistence-cassandra-3.11#cassandra-version-update and check all the points.. Could be that thrift version moved, and is not back in the transitive deps of the 3.11..

Also the list is missing the docker files updates for the v2.. Maybe we can add a PR for the v2 branch once everything is updated on the master.

Copy link
Contributor

@ivansenic ivansenic left a comment

Choose a reason for hiding this comment

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

Stops from merging, due to the #1647 (comment). Same issue exists in the 3.11.12 as well.

@ivansenic
Copy link
Contributor

ivansenic commented Feb 17, 2022

The other failing tests here are reproducible locally..

Step #4 - "cassandra-3.11": [ERROR] Errors: 
Step #4 - "cassandra-3.11": [ERROR]   StargateQueryHandlerTest.authorizeByTokenBatchStatement:936 ? IllegalState Can...
Step #4 - "cassandra-3.11": [ERROR]   IdempotencyAnalyzerTest.shouldReturnIdempotentIfAllStatementsWithinABatchAreIdempotent:102 ? IllegalState
Step #4 - "cassandra-3.11": [ERROR]   IdempotencyAnalyzerTest.shouldReturnNonIdempotentIfAllStatementsWithinABatchAreNonIdempotent:117 ? IllegalState

Seems like a bug due to this changed line in BatchStatement.Parsed. The isFullyQualified() is called, which throws the IllegalArgumentException in the super class CFStatement. I don't really understand how can a batch be fully qualified, this has to be a bug.. This should be confirmed by the BatchStatement.Parsed constructor that calls super with null.

This bug does not exist in the 4.0.2 as they correctly override the isFullyQualified() method.

@tatu-at-datastax
Copy link
Collaborator Author

tatu-at-datastax commented Feb 17, 2022

As per comment on parent issue, some issue(s) could be due to:

https://issues.apache.org/jira/browse/CASSANDRA-17248

which changes behavior wrt fully-qualified prepared statements. Patch is sizable, against 3.0 it's apache/cassandra@242f7f9 (merged upwards)

@tatu-at-datastax
Copy link
Collaborator Author

Completed work from update list, but as to the real problem(s), I am not quite sure how to proceed.

@ivansenic
Copy link
Contributor

Seems that calling QueryProcessor.getStatement instead of QueryProcessor.parseStatement does the trick and batches are correctly prepared in that case.. If tests are green, I will update all the tests existing in all persistence modules to not use QueryProcessor.parseStatement..

Seems we only use this in tests, so should be fine overall..

@ivansenic
Copy link
Contributor

Hmm, OK tests are green, but getStatement does not provide all options as the parseStatement, so I opt in leaving everything else as is. Would this be OK?

There is still the race condition issue here as well, not sure if #1647 would fix that. If so, this should be merged after.

Copy link
Contributor

@ivansenic ivansenic left a comment

Choose a reason for hiding this comment

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

@tatu-at-datastax Approving, but please make sure race condition is fixed in #1647

@tatu-at-datastax tatu-at-datastax merged commit 56a4ef1 into master Feb 23, 2022
@tatu-at-datastax tatu-at-datastax deleted the tatu/1644-upgrade-c_3 branch February 23, 2022 16:32
@tatu-at-datastax
Copy link
Collaborator Author

Still need to resolve the race condition (see #1647) which does also affect C*3.11, before releasing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update C*3, C*4 and DSE dependencies by Stargate
2 participants