quill cassandra for lagom

[WayneWang12]

Problem

I'm using Lagom to build my application. And I find that if quill can support lagom then it maybe perfect. So I find a way to add quill support for lagom.

Solution

Just a little code copy from quill-cassandra and change Cluster to lagom's CassandraSession.

@jilen Please review it. Thank you~

[fwbrasil]

@WayneWang12 thank you for the contribution! What's different about lagom? Is it only the driver interface? are the queries different? We should reuse code.

[WayneWang12]

@fwbrasil Yes you are right. So I've updated my branch. Can you help to review it again? I've extract ed quill-cassandra to quill-cassandra-core, and make quill-cassandra, quill-cassandra-monix and quill-cassandra-lagom to depend on it.

The only difference is that Lagom has its own CassandraSession which is built on akka perssitence and supplies akka stream's Source[T, NotUsed] return type, whick is useful in reactive platform based on Lightbend's frameworks. It's realy useful especially when Lagom have taken Cassandra as their backend persistence layer. However, slick don't support cassandra. So I've made this PR to make Lagom's readside support more convenient. It's mainly for myself to use. If it's not OK to be in quill project, I can create one of my own.

Actually I should create a project for quill-cassandra-akka. But what I need most now is to support Lagom. So I will come it later to complete the akka part for quill.

[WayneWang12]

I'll add unit-test later this day or tommorrow.

[jilen]

I find it possible to share com.datastax.driver.core.Session through SessionProvider of akka persistence.

No need to create different context here.

[WayneWang12]

It seems I just need quill-cassandra-core, then I can extend it to be used by lagom or akka outside quill.

Whether LagomContext is in quill or not is irrelevent. CassandraSessionContext being abstract and independent of Cluster makes it convenient to be extended with all kinds of cassandra drivers.

@fwbrasil If quill-cassandra-lagom is not suitable for quill, I can remove it. Just tell me.

But please extract quill-cassandra-core out. That could save me a lot of effort to use quill in Lagom. Thank you!

[WayneWang12]

Thanks for your reply. I'm on a business trip last few days in Beijing and back to Shanghai this morning. I'll look into this PR soon after my work. Sorry for late response.

[WayneWang12]

@fwbrasil Seems that I've completed this PR. Can you review it again?

Sorry for the delay. Company work is busy in China before Spring Festival.

[WayneWang12]

Finally, all checks have passed 😂 . It's not easy.

[deusaquilus]

Here are some of my general thoughts:

• Why does CassandraMonixContext extend CassandraDatastaxSessionContext? Anything *Datastax should inherit from CassandraAsyncContext, not the reverse.
• We need CassandraSessionContext to do the same kind of thing with ContextEffect[Result] as we did in JdbcContextBase. That way, CassandraLogomContext, CassandraDatastaxSessionContext CassandraMonixContext, CassandraStreamContext, and any other Cassandra* context become practically trivial to implement!
  Just look at JdbcContextBase and then compare PostgresJdbcContextBase versus PostgresMonixJdbcContext. JdbcContextBase does practically everything, the other two just swap in a different ContextEffect.
• CassandraAsyncContext should definitely stay in it's own module and the Datastax and Lagom modules should inherit from it.

I know this would require very big changes and I am prepared to undertake the ContextEffect refactoring part. I am highly against merging this PR until these things are cleaned up, because with this amount of copy-pasting, we risk making Quill Cassandra modules completely unmaintanable.

It is well know with corporate codebases, whenever modules become copy-pasted out of pressing urgency, they tend to drift apart very quickly, often to the point of requiring a gargantuan effort to reconcile or requiring a complete redesign. I think it should be of utmost important to not commit this terrible mistake with the Quill codebase.

Sorry if I sound harsh.

[deusaquilus]

... so. We need a ContextEffect implementation for:

• scala.Future - FutureEffect (for CassandraLagomSessionContext and CassandraAsyncContext)
• akka.stream.scaladsl.Source - AkkaSourceEffect (for the CassandraLagomStreamContext module)
• monix.reactive.Observable - MonixObserveableEffect (for CassandraStreamContext)
• monix.eval.Task - Just Reuse 'Runner' (for CassandraMonixContext)

[deusaquilus]

Here is a sketch what needs to get done in order to have modularity (and DRY!) in the Cassandra module: : #1322

Note how CassandraAsyncContext has a completely trivial implementation delegating all the heavy lifting to CassandraFutureContextEffect. CassandraMonixContext, and CassandraStreamContext (as well as * CassandraLagomStreamContext and * CassandraDatastaxSessionContext should also be reworked to look like that.

[WayneWang12]

@deusaquilus I've read your code. It seems that you want to refactor it to make every effect derived from cluster and session. But Lagom's CassandraSession has already done it for us.

I read Lagom's code carefully this time. They don't expose Cluster to outside. It's hard for me to get Cluster out. So I want to get rid of Datasax and only depend on the core CassandraSessionContext.

Actually every module depends on datasax driver, including monix. It's the base driver. But there exists different kind of wrappers. CassandraDatasaxSessionContext and Lagom's CassandraSession are two of them.

What I have done is to make different wrappers able to reuse the existed code without exposing multiple contexts to users. But the direct dependency of Cluster makes it hard for lagom to complete it. Because Lagom want to hide and unify the configuration in its way. Falling back to direct cluster will make user to maintain two kinds of configuration in their project, one for quill, and one for lagom. But if we use CassandraSession directly, we will not bother of it.

[deusaquilus]

@WayneWang12 I see what you're saying. I think maybe you should rename CassandraDatastaxSessionContext to CassandraClusteredSessionContext to indicate the intent.
Also, I don't see the need to introduce a new quill-cassandra-datastax module. It doesn't have any additional dependencies. Other then that I think it's all good.

@fwbrasil Perhaps trying to do a refactor this big is a bit too ambitious for a feature-PR. Do you agree with my general approach of ContextEffect though?

[WayneWang12]

@deusaquilus At the begining I added a new module named quill-cassandra-core. I was meant to extract all the base features into a single module to be depended by quill-cassandra and quill-cassandra-lagom. I thought it was necessary to hide unneeded classes like CassandraAsyncContext as we introduced new CassandraLagomSessionContext just for Lagom. That's why I extracted quill-cassandra-core and base it to develop quill-cassandra-lagom. @fwbrasil told me to have quill-cassandra and quill-cassandra-datasax, so I did it.

But I still think my initial quill-cassandra-core and quill-cassandra modules are better. Because users needn't change their code if they have already used quill for cassandra. What's your opinion? @deusaquilus

[WayneWang12]

@deusaquilus I've updated this PR. And an extra modification is that in FutureConversion. I think it may be better if we don't introduce extra executors to convert ListenableFuture. Code is as below which I copy from Lagom:

  implicit class ListenableFutureConverter[A](val lf: ListenableFuture[A]) extends AnyVal {
    def asScala(implicit ec: ExecutionContext): Future[A] = {
      val promise = Promise[A]
      lf.addListener(new Runnable {
        def run(): Unit = {
          promise.complete(Try(lf.get()))
          ()
        }
      }, ec.asInstanceOf[Executor])
      promise.future
    }
  }

Previous version will introduce guava's MoreExecutors which is a new thread pool. And it have version conflict as I reported in #1293 while this kind haven't.

[deusaquilus]

@WayneWang12 Looks good! I'll go over it again tomorrow.

[deusaquilus]

Okay, looks good to me.

[deusaquilus]

@fwbrasil What do you think? Are you good with this?

[WayneWang12]

Am I supposed not to click this button??? Sorry I don't know the procedure in Github's new version...

Thanks for your review @deusaquilus ~ Now quill is my first choice for database accessing. Great library you have contributed to the world!

[WayneWang12]

Thanks @fwbrasil for accepting my pull request 😄