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

Migrates to Datastax Driver v4 #3246

Merged
merged 1 commit into from
Oct 21, 2020
Merged

Migrates to Datastax Driver v4 #3246

merged 1 commit into from
Oct 21, 2020

Conversation

codefromthecrypt
Copy link
Member

@codefromthecrypt codefromthecrypt commented Oct 18, 2020 •
edited
Loading

This migrates to the Netty-based Datastax Driver v4, which no longer
uses guava. This is very different library architecture, most obvious in
how configuration is managed. This migration tries to be as conventional
as possible while retaining defaults as close to as they were as
possible.

Special thanks to @adutra and @olim7t who helped with translation things on the https://groups.google.com/a/lists.datastax.com/g/java-driver-user list

cc @openzipkin/cassandra

Fixes #2405
Fixes #1276

codefromthecrypt
codefromthecrypt commented Oct 18, 2020
View reviewed changes
docker/storage/cassandra/install.sh Outdated
@@ -26,7 +26,9 @@ wget -qO- $APACHE_MIRROR/cassandra/$CASSANDRA_VERSION/apache-cassandra-$CASSANDR
--strip=1

# Merge in our custom configuration
sed -i '/enable_user_defined_functions: false/cenable_user_defined_functions: true' conf/cassandra.yaml
for feature in enable_user_defined_functions enable_sasi_indexes; do
Copy link
Member Author

Choose a reason for hiding this comment

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

while this enables it, heh we still get server side warnings 'SASI indexes are experimental and are not recommended for production use.'

codefromthecrypt
codefromthecrypt commented Oct 18, 2020
View reviewed changes
zipkin-server/pom.xml
<optional>true</optional>
</dependency>
<!-- <dependency>-->
<!-- <groupId>io.zipkin.brave.cassandra</groupId>-->
Copy link
Member Author

Choose a reason for hiding this comment

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

something else to upgrade now :)

codefromthecrypt
codefromthecrypt commented Oct 18, 2020
View reviewed changes
...r/src/main/java/zipkin2/server/internal/cassandra3/ZipkinCassandra3StorageConfiguration.java
Tracing tracing = beanFactory.getBean(Tracing.class);
return (SessionFactory) storage -> TracingSession.create(tracing, delegate.create(storage));
}
//if (bean instanceof SessionFactory && beanFactory.containsBean("tracing")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@jeqo this is the style you can use for kafka (not the commenting out :P)

codefromthecrypt
codefromthecrypt commented Oct 18, 2020
View reviewed changes
zipkin-storage/cassandra-v1/src/main/java/zipkin2/storage/cassandra/v1/CassandraUtil.java
static List<Date> getDays(long endTs, @Nullable Long lookback) {
List<Date> result = new ArrayList<>();
static List<Instant> getDays(long endTs, @Nullable Long lookback) {
List<Instant> result = new ArrayList<>();
Copy link
Member Author

Choose a reason for hiding this comment

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

instant -> timestamp column

@codefromthecrypt
Copy link
Member Author

Special thanks to @adutra and @olim7t who helped with translation things on the https://groups.google.com/a/lists.datastax.com/g/java-driver-user list

@jorgheymans jorgheymans mentioned this pull request Oct 18, 2020
Open
michaelsembwever
michaelsembwever approved these changes Oct 18, 2020
View reviewed changes
Copy link
Member

@michaelsembwever michaelsembwever left a comment

Choose a reason for hiding this comment

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

PR looks good Adrian!

It's a bit big to go through line by line, but definitely the most of it looks good. I'm surprised you're still keeping the old cassandra storage.

@codefromthecrypt
Copy link
Member Author

NOTE: biggest thing here is that CASSANDRA_LOCAL_DC env must be set to a valid value as the driver now requires it. We fall back to Cassandra's default of "datacenter1" when unset.

@codefromthecrypt
Copy link
Member Author

I'm surprised you're still keeping the old cassandra storage.

@michaelsembwever This is mainly because cassandra3 relies on SASI which is still marked experimental. We can decide to unhook from that I suppose, and just implement the same indexing style we used before (except more efficient as in v2 model now)...

@codefromthecrypt
Copy link
Member Author

"cassandra3" looks working so far. kinks in "cassandra" not many. we are sorely missing docker tests :P will get to that in a week across the board.

@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Oct 19, 2020 •
edited
Loading

Unintuitively, even after trimming jars, this results in a larger server binary than when we didn't share netty. The reason is querybuilder includes a massive dep on shaded guava (2.8m). I will remove that in a later PR.

@codefromthecrypt
Copy link
Member Author

schema takes about 10s to do now (currently happens per test).. which is intensely slowing tests probably beyond what travis will allow.

@codefromthecrypt
Copy link
Member Author

yep.. "The job exceeded the maximum time limit for jobs, and has been terminated."

@codefromthecrypt
Copy link
Member Author

for cassandra pros, even locally it seems excessive time to install schema. I will try to re-use the schema as we can't afford a delay like this.

2020-10-19 19:06:50:811 [main] INFO zipkin2.storage.cassandra.Schema - Installing schema /zipkin2-schema.cql for keyspace skipsredundantindexinginatrace
2020-10-19 19:06:56:385 [main] INFO zipkin2.storage.cassandra.Schema - Installing indexes /zipkin2-schema-indexes.cql for keyspace skipsredundantindexinginatrace
2020-10-19 19:07:07:283 [main] DEBUG zipkin2.storage.cassandra.Schema - Registering endpoint and annotation UDTs to keyspace skipsredundantindexinginatrace
2020-10-19 19:07:07:285 [main] INFO zipkin2.storage.cassandra.Schema - done

@codefromthecrypt
Copy link
Member Author

Tests except ITEnsureSchema use the same keyspace now. should be a lot quicker

@codefromthecrypt
Copy link
Member Author

much faster, but cassandra-v1 has a few flakey tests, now. I'll look into them tommorrow

@codefromthecrypt
Copy link
Member Author

I have some refactoring to isolate heavy tests across the board. I'll move that to another PR.

@codefromthecrypt
Copy link
Member Author

I think I have things finally stable now and pulled out #3249 . once that's in I'll rebase this (removing the heft of the test infra changes)

Migrates to Datastax Driver v4
ce60e5b 
This migrates to the Netty-based Datastax Driver v4, which no longer
uses guava. This is very different library architecture, most obvious in
how configuration is managed. This migration tries to be as conventional
as possible while retaining defaults as close to as they were as
possible.
@codefromthecrypt
Copy link
Member Author

yay green! Next PR will remove the querybuilder dep so that our server binary isn't larger.

@codefromthecrypt codefromthecrypt merged commit 9674c46 into master Oct 21, 2020
@codefromthecrypt codefromthecrypt deleted the cassandra3d branch October 21, 2020 03:36
@codefromthecrypt
Copy link
Member Author

#3250 removes the querybuilder dep, but the core lib still seems to depend on a massive shaded guava lib https://groups.google.com/u/1/a/lists.datastax.com/g/java-driver-user/c/aN7NkxX_yQk

mauriziocescon added a commit to mauriziocescon/zipkin that referenced this pull request Nov 16, 2020
@mauriziocescon
Merge branch '2_22_0' into prettifier
a34e004 
* 2_22_0: (108 commits)
  [maven-release-plugin] prepare release 2.22.0
  Hardens integration tests, fixes small bug (openzipkin#3258)
  Deprecates Cassandra v1 schema for removal in Zipkin 2.23 (openzipkin#3254)
  Update Armeria to 1.2.0 and Netty to 4.1.53 (openzipkin#3257)
  Removes JMX dependency from our docker configuration (openzipkin#3252)
  Quiets startup logging (openzipkin#3253)
  Removes Cassandra querybuilder dependency (openzipkin#3250)
  Migrates to Datastax Driver v4 (openzipkin#3246)
  Refactors integration tests to be more isolated (openzipkin#3249)
  Adds workaround to missed decorator route (openzipkin#3245)
  disallow trace requests (openzipkin#3239)
  Refactors Cassandra queries so they are cheaper and easier to migrate (openzipkin#3243)
  fix docker hook CWD
  Builds builder with same script as other images (openzipkin#3242)
  Removes last clutter from zipkin startup (openzipkin#3240)
  Stops docker-compose from being mistaken as a workflow (openzipkin#3241)
  fix master
  Finishes Docker refactoring (openzipkin#3238)
  Bumps versions and applies failsafe workaround (openzipkin#3235)
  Fix a bug in the unit of duration (openzipkin#3234)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelsembwever michaelsembwever michaelsembwever approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update to Datastax Cassandra Driver v4 Get rid of zipkin3_udts keyspace
2 participants
@codefromthecrypt @michaelsembwever