Implementing storehaus with Postgres backend by ponkin · Pull Request #331 · twitter/storehaus

ponkin · 2016-10-25T08:18:06Z

Hi everybody. Here is storehouse implementation for postgres.
Need your review and comments.

johnynek

Thanks for the contribution. I made some suggestions. Let me know if you have any questions.

johnynek · 2016-10-25T20:21:00Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/Read.scala

+  *
+  * Created by ponkin on 10/20/16.
+  */
+trait Read[A] {


can we instead follow the style of the rest of the project and use Injection[A, PostgresValue] here instead?

I was using Injections at first but it went me to many ugly implicit arguments like
PostgresStoreK, V(implicit inj1: Injection[K, PostgresValue], inj2: Injection[V, PostgresValue], inj3: Injection[K, Element], inj1: Injection[V, Element]) - and also expose details of implementation( Element is object from finagle-roc postgres driver). May be you can point me some example, how to do it right?

you could do in your package:

type PostgresValueConverter[A] = Injection[A, PostgresValue]

then in the object PostgresValue { ... define the implicits and it should be pretty clean and pick them up automatically.

johnynek · 2016-10-25T20:22:00Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/PostgresStore.scala

+      case (key, Some(value)) => upsertSQL(List((key, value)))
+      case (key, None) => deleteSQL(List(key))
+    }
+    println(sql)


I don't think we should have the println around.

johnynek · 2016-10-25T20:23:52Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/PostgresStore.scala

+      }
+      ks.foldLeft(Map.empty[K1, Future[Option[V]]]) { (acc, key) =>
+        acc + (key -> reqMap.map(_.get(key)))
+      }


ks.iterator.map { key => (key, reqMap.map(_.get(key))) }.toMap

would be more direct than building the map yourself (and maybe faster since it can use a MapBuilder).

johnynek · 2016-10-25T20:26:29Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/PostgresStore.scala

+    s"DELETE FROM $table WHERE $kCol"
+
+  override def get(key: K): Future[Option[V]] = {
+    val result = run(selectSQL(List(key)))


can we pass a limit here? I'm also worried that we may have more than one item. I would rather limit to 2, then do:

result.flatMap { case Nil => Future.None case h :: Nil => Future.value(Read[V].read(h.get(Symbol(vCol)))) case other => Future.exception(new Exception(s"for key: $key expected 0 or 1 items, found: $other")

johnynek · 2016-10-25T20:28:19Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/PostgresValue.scala

+  *
+  * Created by ponkin on 10/18/16.
+  */
+sealed trait PostgresValue


why the extra wrapper on Element?

to change Postgres driver(finagle-roc) in future(if we we want to).

I see. That's fine if we are going to automatically handle the conversions I think.

johnynek · 2016-10-25T20:29:03Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/PostgresStore.scala

+      Map.empty
+    } else {
+      val sql = doMultiPut(kvs)
+      println(sql)


let's remove this.

johnynek · 2016-10-25T20:29:23Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/PostgresStore.scala

+    multiPutSQL(toUpsert, toDelete.toList)
+  }
+
+  override def close(t: Time): Future[Unit] = {


don't need { } for single expression methods.

johnynek · 2016-10-25T20:30:11Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/PostgresStore.scala

+    new PostgresStore[K, V](client, table, kCol, vCol)
+}
+
+class PostgresStore[K: Show : Read, V: Show : Read]


would it be too much work to add a PostgresMergeableStore that does merge in a transaction? That would be nice to have for several use cases.

Sure, I will do it!

Well, that part turns out to be kind of tricky). How can we implement merge since we use async driver( separate calls client.query("BEGIN") and client.query("COMMIT;") can lead to unknown result, since some requests can be made simultaneously and can be added in transaction). I do not know ho to tie Futures to particular connection. Any ideas? We can use optimistic locking but we need additional column to keep lock, like timestamp or version.

johnynek · 2016-11-03T18:13:04Z

build.sbt

    "com.github.finagle" %% "roc-core"  % "0.0.5",
-    "com.github.finagle" %% "roc-types" % "0.0.5"
+    "com.github.finagle" %% "roc-types" % "0.0.5",
+    "com.chuusai"        %% "shapeless" % "2.3.2"


do we need this? I don't see where.

johnynek · 2016-11-03T18:13:15Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/MergeablePostgresStore.scala

+/**
+  * Created by ponkin on 10/26/16.
+  */
+class MergeablePostgresStore[V] {


why do we need this?

johnynek · 2016-11-03T18:17:28Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/package.scala

-  implicit val boolRead = new Read[Boolean] {
-    override def read(v: PostgresValue): Boolean = v match {
-      case RocPostgresValue(e) => e.as[Boolean]
+  implicit val boolInjection= new OneWayInjection[Boolean] {


okay, I'm really sorry. I overlooked that your Read and Show were not inverses of each other. One is going to String the other is coming from a PostgresValue. I wish I had noticed that.

I'd actually prefer the way you had it to throwing exceptions. I mistakenly thought we could combine Read and Show into Injection but not the way you have the code.

What about a direct trait like:

trait PostgresCodec[A] { def encode(a: A): String def decode(p: PostgresValue): Try[A] }

I don't see a need to decouple these since they are so special purpose. I thought Injection would do, but the types are not the same on encode and decode.

ponkin · 2016-11-13T20:19:02Z

@johnynek Sorry for long delay. I added MergeablePostgresStore and tests for it. Also I rewrote all with Injections. I was trying to keep overall style. please review)

johnynek · 2016-11-22T17:33:40Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/MergeablePostgresStore.scala

+   * All values with the same key will be merged with Semigroup[V2].plus method
+   */
+  private def mergeWithSemigroup[K1 <: K](m1: Map[K, V], m2:  Map[K1, V]): Map[K1, V] = {
+    m2.keySet.iterator.map{ k2 =>


foldLeft is generally faster because you may get some sharing:

val smaller = if (m1.size < m2.size) m1 else m2 val bigger = if (m1.size < m2.size) m2 else m1 smaller.foldLeft(bigger) { case (m, (k, v)) => val newV = Semigroup.maybePlus(v, m.get(k)) m + ((k, newV)) }

may need to bump the algebird dependency to get maybePlus, but that is fine.

johnynek · 2016-11-22T17:37:42Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/PostgresStore.scala

+                                         forUpdate: Boolean = false)
+                                        (implicit cli: Client): Future[Map[K, V]] =
+    if (ks.isEmpty) {
+      Future(Map.empty)


Future.value(Map.empty) is better since it avoids the call-by-name parameter, and since that is a constant, you might save it as a private[this] val and avoid the reallocation.

johnynek · 2016-11-22T17:42:40Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/package.scala

+
+  trait OneWayInjection[T] extends Injection[T, PostgresValue] {
+    override def apply(t: T): PostgresValue =
+      throw IllegalConversionException(


can we avoid the throwing? I know we don't use it, but I really don't want to add code that throws unless we have no other way.

Here I can see two ways:

just add the code to wrap the primitives below in PostgresValue, which does not seem hard.

make a type-class that only goes in one direction.

johnynek · 2016-11-22T17:44:26Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/PostgresStore.scala

+
+  private val toV: PostgresValue => V = vInj.invert(_) match {
+    case Success(v) => v
+    case Failure(e) => throw IllegalConversionException(e.getMessage)


why do we need to throw here? Why not just put this in the future?

private def toV(p: PostgresValue): Future[V] = vInj.invert(p) match { case Success(v) => Future.value(v) case Failure(e) => Future.exception(e) }

then use toV(v).flatMap { ... rather than throwing?

johnynek · 2016-11-22T17:49:39Z

storehaus-postgresql/src/main/scala/com/twitter/storehaus/postgres/package.scala

+    }
+  }
+
+  def toEscString(a: Any): String = a match {


I don't like this approach since we have no guarantee that toString is going to work. Why can't use a full injection to PostgresValue then use some safe mechanism to go from Value[Any] to String that can be interpolated in a SQL statement. I assume the Value wrapper has some support for that, no?

CLAassistant · 2019-07-18T15:12:09Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ Alexey Ponkin
❌ ponkin

Alexey Ponkin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Alexey Ponkin added 2 commits October 25, 2016 11:15

Implementting storehaus with Postgres backend

feffb43

Adding module storehuas-postgres to build.sbt

aa80d41

johnynek requested changes Oct 25, 2016

View reviewed changes

Alexey Ponkin added 4 commits October 26, 2016 13:33

Adding MergeablePostgresStore and Injections instead of Show Read

c26eaf8

Create seperate bransh for MergeableStore

d772edc

Scalaz functors and kleisli

316c640

Added MergeablePostgresStoreProperties, need implementation

e6d6dee

johnynek requested changes Nov 3, 2016

View reviewed changes

Tests are working

e6122e7

ponkin added 2 commits November 19, 2016 18:08

Fixing MergeablePostgresStoreProperties test bugs

2983f92

Fixing travis.ci to support postgres DB

ee9df7e

johnynek reviewed Nov 22, 2016

View reviewed changes

Conversation

ponkin commented Oct 25, 2016

Uh oh!

johnynek left a comment

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!

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!

ponkin commented Nov 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

CLAassistant commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ponkin commented Nov 13, 2016 •

edited

Loading

CLAassistant commented Jul 18, 2019 •

edited

Loading