Skip to content

Upgrade Cassandra Driver to 4.3.1 #26

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeff-blaisdell
Copy link
Member

@jeff-blaisdell jeff-blaisdell commented Dec 27, 2019

This MR upgrades the Cassandra Migration project to use the latest Cassandra Java driver (4.3.1). It contains many breaking changes from the 3.2.0 version currently in use. As a result of the upgrade the hard dependency on Guava 19.0 has been dropped. The 4.3.1 variant of Cassandra driver shades this to com.datastax.oss.driver.shaded.guava.common

See:
https://github.com/datastax/java-driver/tree/4.x/upgrade_guide

and

https://docs.datastax.com/en/developer/java-driver/4.3/

This bumps version to 0.2.0. Additionally, I made a 0.1.x-master to track previous version.

@codecov
Copy link

codecov bot commented Dec 27, 2019

Codecov Report

Merging #26 into master will increase coverage by 2.3%.
The diff coverage is 82.89%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master      #26     +/-   ##
===========================================
+ Coverage     52.95%   55.25%   +2.3%     
- Complexity       82       84      +2     
===========================================
  Files            11       11             
  Lines           508      552     +44     
  Branches         46       46             
===========================================
+ Hits            269      305     +36     
- Misses          215      221      +6     
- Partials         24       26      +2
Impacted Files Coverage Δ Complexity Δ
src/main/java/smartthings/util/Util.java 50% <ø> (ø) 5 <0> (ø) ⬇️
...in/java/smartthings/migration/MigrationRunner.java 58.62% <0%> (ø) 7 <0> (ø) ⬇️
...main/java/smartthings/cassandra/CassandraLock.java 87.34% <100%> (+5.86%) 17 <7> (ø) ⬇️
...ava/smartthings/migration/MigrationParameters.java 47.61% <50%> (-1.29%) 17 <2> (+1)
...ava/smartthings/cassandra/CassandraConnection.java 57.97% <80%> (+1.55%) 32 <4> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef9874d...e8bb261. Read the comment docs.

testCompile 'org.objenesis:objenesis:2.4'
testRuntime 'io.netty:netty-all:4.1.4.Final'
testRuntime 'ch.qos.logback:logback-classic:1.1.7'
testCompile 'org.cassandraunit:cassandra-unit:4.2.2.0-SNAPSHOT'
Copy link
Member Author

Choose a reason for hiding this comment

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

);

updateLock = session.prepare(
SimpleStatement.newInstance("UPDATE databasechangelock USING TTL :ttl SET lockedby = :owner WHERE id = :lockId IF lockedby = :ifowner")
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit obnoxious. All the bind variable - set by name methods appear only to support setting the first occurrence.
See:
https://github.com/datastax/java-driver/blob/4.x/core/src/main/java/com/datastax/oss/driver/api/core/data/SettableByName.java

Best solution I've determined so far is just make each bind name unique.. Couldn't find a method/impl that automagically set them all. If you leave it the same it results in a RuntimeException essentially saying you have unset binds.

ResultSet rs = session.execute(insertLock.bind().setInt("lockId", lockId)
.setInt("ttl", ttl).setString("owner", owner));
ResultSet rs = session.execute(
insertLock.boundStatementBuilder()
Copy link
Member Author

Choose a reason for hiding this comment

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

Switched bind to boundStatementBuilder to reduce cost of data copy due to immutable internal implementation.

@@ -586,3 +586,5 @@ encryption_options:
# cipher_suites: [TLS_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_256_CBC_SHA]

hints_directory: /tmp/hints

cdc_raw_directory: build/embeddedCassandra/raw
Copy link
Member Author

Choose a reason for hiding this comment

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

Required to start cassandra-unit's embedded cass after version upgrade.

assert column.keyspace == keyspace
[column.name, row.getString(column.name)]
assert column.keyspace.asCql(true) == keyspace
[column.name.asCql(true), row.getString(column.name)]
Copy link
Member Author

Choose a reason for hiding this comment

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

column.keyspace and column.name no longer strings. Later cassandra driver introduces a wrapper type CqlIdentifer.

https://github.com/datastax/java-driver/blob/4.x/core/src/main/java/com/datastax/oss/driver/api/core/CqlIdentifier.java#L118

Cluster.Builder builder = Cluster.builder().addContactPoint(host).withPort(port).withMaxSchemaAgreementWaitSeconds(20).withQueryOptions(queryOptions);
CqlSessionBuilder builder = CqlSession.builder()
.addContactPoint(new InetSocketAddress(host, port))
.withLocalDatacenter(localDatacenter)
Copy link
Member Author

@jeff-blaisdell jeff-blaisdell Dec 27, 2019

Choose a reason for hiding this comment

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

Cassandra Driver 4.3 will fail to startup when a host / port is specified without also setting the local datacenter field. Added it as a settable parameter w/ a default of "datacenter1" which matches the drivers default value.

This has more details:
https://docs.datastax.com/en/developer/java-driver/4.2/manual/core/load_balancing/

Reading of the docs seems that as we upgrade to the 4.x drivers we should be setting this field in higher environments.. Though I'm a little unsure to what. Could use some feedback here from those closer to Cassandra.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants