Allow customization of C* cluster and session creation. by llinder · Pull Request #14 · SmartThingsOSS/ratpack-cassandra

llinder · 2017-02-02T20:14:39Z

Expose C* cluster and session instances directly through Guice
Allow overriding cluster creation through module constructor methods
Uses a factory pattern for the session to allow overriding how the session is exposed to Guice

This deviates from the current pattern where the C* session is created by a Ratpack Service onStart method. Downside here is that things will block when the Session is created by Guice.

/cc @beckje01 @charliek @jeff-blaisdell

llinder · 2017-02-02T20:21:58Z

ratpack-cassandra/src/main/java/smartthings/ratpack/cassandra/CassandraSessionFactory.java

+	@Override public Session get() {
+		return (config.keyspace != null && !"".equals(config.keyspace)) ?
+				cluster.connect(config.keyspace) :
+				cluster.connect();


This was previously done in the Service onStart method. Moving it here makes it simple to change to a different Session implementation for tracing.

But this means the app will start without being able to connect to C*

Its probably my lack of understanding of Ratpack but I don't really follow what you mean. If another component depends on a Session through dependency injection then Guice would refer to this singleton to create that. By the nature of depending on a Session it would be available. If cluster.connect() fails in this Guice provider then the app would fail to start. As far as I understand it doesn't change the DI contract from the previous pattern except that the session was previously created from an onStart method instead of the DI framework.

jeff-blaisdell · 2017-02-03T22:38:07Z

ratpack-cassandra/src/main/java/smartthings/ratpack/cassandra/CassandraModule.java


 	@Override
 	protected void configure() {
+		bind(Session.class).to(DefaultSession.class).in(Scopes.SINGLETON);


Rather than subclass this module another possible option is to use the Guice OptionalBinder. I hadn't used it myself until today, but it seems pretty convenient. Then you can do it all in your own module rather than creating a new subclass.

OptionalBinder.newOptionalBinder(binder(), AccountService.class) .setDefault().to(DefaultAccountService.class).in(Scopes.SINGLETON);

But I guess that would require you to move the Session creation logic into some sort of factory service that could be overridden. Which maybe then it's more or less equivalent.

Thats an interesting option, thanks for suggesting it. Are there any examples of this pattern being used somewhere? We could certainly give the OptionBinder + factory service a try if it seems preferable.

Your call, what you have is good as well. I used the optional binder pattern in our internal route common project. I personally think it makes for a nice extension point, but what you have does as well.

Usage looks like this...
You define a default in a lib module like this:

OptionalBinder.newOptionalBinder(binder(), AccountService.class) .setDefault().to(DefaultAccountService.class).in(Scopes.SINGLETON);

And then you can override it in a project like this:

OptionalBinder.newOptionalBinder(binder(), AccountService.class) .setBinding().to(ZipkinAccountService.class).in(Scopes.SINGLETON);

- Expose C* cluster and session instances directly through Guice - Allow overriding cluster creation through module constructor methods - Uses a factory pattern for the session to allow overriding how the session is exposed to Guice

llinder · 2017-02-06T00:32:02Z

ratpack-cassandra/src/main/java/smartthings/ratpack/cassandra/RatpackSession.java

+
+	Promise<ResultSet> executePromise(String query, Map<String, Object> values);
+
+	Promise<ResultSet> executePromise(Statement statement);


I noticed this method on the previous CassandraService class. Seems useful so I created Promise based methods for all of the Session async methods.

beckje01 · 2017-02-13T22:36:18Z

...cassandra-migrate/src/main/java/smartthings/ratpack/cassandra/CassandraMigrationService.java

 import smartthings.migration.MigrationRunner;

-@DependsOn(CassandraService.class)
+@DependsOn(Session.class)


Does this work?

I believe this works as long as the provided implementation extends @Service. It would be good to test that part more though.

charliek · 2017-04-24T18:17:12Z

build.gradle

+				username smartThingsUserName
+				password smartThingsPassword
+			}
+			url "https://smartthings.artifactoryonline.com/smartthings/libs-release-local"


What is this needed for? I don't think we want the OSS repos to pull from our internal artifactory.

It should be here. Thanks for catching it.

jkwon-kim · 2017-04-25T03:12:10Z

ratpack-cassandra-tracing/src/main/java/smartthings/ratpack/cassandra/zipkin/TracedSession.java

+	protected Session createDelegate() {
+		Session session = (keyspace != null && !keyspace.equals("")) ? cluster.connect(keyspace) : cluster.connect();
+
+		return smartthings.brave.cassandra.TracedSession.create(session, brave, config.getServiceName());


@llinder, the resultset callback works properly with Ratpack when we use this smartthings.brave.cassandra.TracedSession? (https://github.com/SmartThingsOSS/smartthings-brave/blob/master/brave-cassandra-latencytracker/src/main/java/smartthings/brave/cassandra/TracedSession.java#L122).

Here's the stacktrace Brian is seeing:

SEVERE: RuntimeException while executing runnable smartthings.brave.cassandra.TracedSession$TracedResultSetFuture$$Lambda$1@30335d5c with executor MoreExecutors.directExecutor() ratpack.exec.UnmanagedThreadException: Operation attempted on non Ratpack managed thread 'cluster1-nio-worker-2' at ratpack.exec.internal.DefaultExecution.require(DefaultExecution.java:104) at ratpack.exec.Execution.current(Execution.java:81) at ratpack.zipkin.internal.RatpackServerClientLocalSpanState.setCurrentServerSpan(RatpackServerClientLocalSpanState.java:104) at com.github.kristofa.brave.ServerSpanThreadBinder.setCurrentSpan(ServerSpanThreadBinder.java:52) at smartthings.brave.cassandra.TracedSession$TracedResultSetFuture.lambda$addListener$0(TracedSession.java:404) at smartthings.brave.cassandra.TracedSession$TracedResultSetFuture$$Lambda$1.run(Unknown Source) at com.google.common.util.concurrent.MoreExecutors$DirectExecutor.execute(MoreExecutors.java:456) at com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:817) at com.google.common.util.concurrent.AbstractFuture.complete(AbstractFuture.java:753) at com.google.common.util.concurrent.AbstractFuture.set(AbstractFuture.java:613) at com.datastax.driver.core.DefaultResultSetFuture.onSet(DefaultResultSetFuture.java:174) at com.datastax.driver.core.RequestHandler.setFinalResult(RequestHandler.java:174) at com.datastax.driver.core.RequestHandler.access$2600(RequestHandler.java:43) at com.datastax.driver.core.RequestHandler$SpeculativeExecution.setFinalResult(RequestHandler.java:793) at com.datastax.driver.core.RequestHandler$SpeculativeExecution.onSet(RequestHandler.java:496) at com.datastax.driver.core.Connection$Dispatcher.channelRead0(Connection.java:1012) at com.datastax.driver.core.Connection$Dispatcher.channelRead0(Connection.java:935)

From a really quick look, my guess is that the traced version has brave internals operating in a non-ratpack way. I think the problem might be here:

https://github.com/SmartThingsOSS/ratpack-cassandra/blob/master/ratpack-cassandra/src/main/java/smartthings/ratpack/cassandra/CassandraService.java#L105

I wonder if changing to a Blocking.get would solve for both (Traced and Non-Traced).

I think the problem is that the current span is being injected into the Ratpack registry from a thread spawned by the Cassandra driver.

What we will need to figure out is how to call ServerSpanThreadBinder.setCurrentSpan(span) from a Ratpack managed thread.

We might want to consider using a different trace context. I can't think of a good reason it needs to interact with the Ratpack lifecycle at that level. Even a thread local context might be suitable for our needs.

I will try to do some investigation work later today.

llinder commented Feb 2, 2017

View reviewed changes

llinder force-pushed the sessionoverride branch from e554bc8 to 99590a8 Compare February 3, 2017 21:39

jeff-blaisdell reviewed Feb 3, 2017

View reviewed changes

llinder force-pushed the sessionoverride branch from 99590a8 to b0fe5ee Compare February 3, 2017 22:44

llinder force-pushed the sessionoverride branch from b0fe5ee to a656093 Compare February 3, 2017 23:11

llinder commented Feb 6, 2017

View reviewed changes

beckje01 reviewed Feb 13, 2017

View reviewed changes

charliek reviewed Apr 24, 2017

View reviewed changes

Add Cassandra tracing module

6dafbc6

llinder force-pushed the sessionoverride branch from e7a6ba2 to 6dafbc6 Compare April 24, 2017 18:25

jkwon-kim reviewed Apr 25, 2017

View reviewed changes


		Promise<ResultSet> executePromise(String query, Map<String, Object> values);

		Promise<ResultSet> executePromise(Statement statement);

Comments

Conversation

llinder commented Feb 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llinder Feb 3, 2017 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

llinder Feb 3, 2017 •

edited

Loading