Allow customization of C* cluster and session creation. #14
Conversation
| @Override public Session get() { | ||
| return (config.keyspace != null && !"".equals(config.keyspace)) ? | ||
| cluster.connect(config.keyspace) : | ||
| cluster.connect(); |
There was a problem hiding this comment.
This was previously done in the Service onStart method. Moving it here makes it simple to change to a different Session implementation for tracing.
There was a problem hiding this comment.
But this means the app will start without being able to connect to C*
There was a problem hiding this comment.
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.
llinder
force-pushed
the
sessionoverride
branch
from
e554bc8 to
99590a8
Compare
| @@ -139,8 +161,93 @@ public void setPassword(String password) { | |||
|
|
|||
| @Override | |||
| protected void configure() { | |||
| bind(Session.class).to(DefaultSession.class).in(Scopes.SINGLETON); | |||
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
99590a8 to
b0fe5ee
Compare
- 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
force-pushed
the
sessionoverride
branch
from
b0fe5ee to
a656093
Compare
|
|
||
| Promise<ResultSet> executePromise(String query, Map<String, Object> values); | ||
|
|
||
| Promise<ResultSet> executePromise(Statement statement); |
There was a problem hiding this comment.
I noticed this method on the previous CassandraService class. Seems useful so I created Promise based methods for all of the Session async methods.
| @@ -10,7 +10,7 @@ | |||
| import smartthings.migration.MigrationParameters; | |||
| import smartthings.migration.MigrationRunner; | |||
|
|
|||
| @DependsOn(CassandraService.class) | |||
| @DependsOn(Session.class) | |||
There was a problem hiding this comment.
I believe this works as long as the provided implementation extends @Service. It would be good to test that part more though.
build.gradle
Outdated
| username smartThingsUserName | ||
| password smartThingsPassword | ||
| } | ||
| url "https://smartthings.artifactoryonline.com/smartthings/libs-release-local" |
There was a problem hiding this comment.
What is this needed for? I don't think we want the OSS repos to pull from our internal artifactory.
There was a problem hiding this comment.
It should be here. Thanks for catching it.
llinder
force-pushed
the
sessionoverride
branch
from
e7a6ba2 to
6dafbc6
Compare
| protected Session createDelegate() { | ||
| Session session = (keyspace != null && !keyspace.equals("")) ? cluster.connect(keyspace) : cluster.connect(); | ||
|
|
||
| return smartthings.brave.cassandra.TracedSession.create(session, brave, config.getServiceName()); |
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
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:
I wonder if changing to a Blocking.get would solve for both (Traced and Non-Traced).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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