Front end rehaul #212

gurghet · 2020-12-28T20:13:11Z

This separates database fetching in a separate module.
It adds a local folder for quick testing.

gurghet · 2020-12-28T20:23:59Z

example/src/main/scala/tamer/example/Main.scala

+    log  <- logTask
+    _    <- log.info("Starting tamer...")
+    boot <- UIO(Instant.now())
+    setup = tamer.db.mkSetup(ts =>


A more general setup is currently missing, this particular setup is constrained to use Datable types and leverages the assumption that we are working with time series.

sirocchj

This is a great step forward, added a few comments on the parts I could spot on mobile and/or I'm less convinced of

sirocchj · 2020-12-28T21:12:54Z

build.sbt

@@ -19,6 +19,7 @@ lazy val V = new {
  val scalatest     = "3.2.3"
  val silencer      = "1.7.1"
  val zio           = "1.0.3"
+  val `zio-s3`      = "latest.integration"


Coming soon? 🙂

Suggested change

val `zio-s3` = "latest.integration"

val `zio-s3` = "0.2.5"

https://github.com/zio/zio-s3/releases/tag/v0.2.5

core/src/main/scala/tamer/Setup.scala

sirocchj · 2020-12-28T21:33:27Z

doobie/src/main/scala/tamer/db/package.scala

+      if (queryResult.results.isEmpty) timeSegment.to.plus(tumblingStep).orNow.map(TimeSegment(timeSegment.from, _))
+      else {
+        val mostRecent = queryResult.results.max.instant
+        mostRecent.plus(tumblingStep).orNow.map(TimeSegment(mostRecent, _))


I'm afraid that doing it here would prevent clients from doing things like: well, once I'm 2/3 seconds wall clock time from current time (yes, they are subject to skew so it's a big assumption) I'm going to back off and run queries slower (see #44 (comment))

I'm sure we could work around this but in general removing client control from what State makes sense to them and constrain it to be a TimeSegment might be counter productive (e.g. a client that always needs to pull the entire table or one that needs to scan through a sortable primary key). Alternatively, we provide a factory for multiple common cases (this would be one) and allow one that is much like the original (leaving the movement of the cursor completely to the caller)

Yes this is the first part of the second scenario. I wanted to work from a specific practical case to establish a baseline. I’m going to build the more generic setup next and it will be feature parity. The reason is to optimize for (what I think will be) the common case, and then give the user freedom to write his own function if he likes.

doobie/src/main/scala/tamer/db/package.scala

sirocchj · 2020-12-28T21:41:37Z

example/src/main/scala/tamer/example/Main.scala

-          val max = values.map(_.modifiedAt).max // if we can't order in the query we need to do it here...
-          max.plus5Minutes.orNow.map(State(max, _))
-      }
+object Main extends zio.App {


Feels like we could still be composing this more minimally here in the example. There should be a way to provide all the below in the core module, bar the db parts.

Yes, I didn't spend much time on this and I'll leave tidying up the layers last or even in another PR. I expect it to be rather mechanical.

also will inline keyExtract and logTask to make it less crowded

…t-end-rehaul

Co-authored-by: Julien Jean Paul Sirocchi <[email protected]>

gurghet · 2020-12-30T10:49:34Z

This should have feature parity with the old front-end. Is there anything missing? If nothing missing I’ll do a general clean-up and update the README

sirocchj

With the caveat that the best parts of both Main and MainGeneralized could (and likely should) go back to core (so all Tamer apps are initialised the same way, all log the same way, etc and in general there's less boilerplate for the end user to have to write), this is a huge 👍

We can later decide whether using a typeclass-like approach for the hashable state is better/cleaner

sirocchj · 2020-12-30T19:11:18Z

doobie/src/main/scala/tamer/db/ConfigDb.scala

+  }
+
+  val live: Layer[TamerError, Has[Db] with Has[Query]] = ZLayer.fromEffectMany {
+    deleteMeConfigValue.load[Task].refineToOrDie[ConfigException].mapError(ce => TamerError(ce.error.redacted.show, ce)).map {


This was to be deleted? Maybe just folding the two things into one config will do too

Oh yeah, I eventually renamed it but the IDE refactoring algo messed up here. I'll rename it for now. The db module is on it's own now so it can be refactored freely in another PR.

gurghet · 2021-01-01T20:53:41Z

As a first iteration I think this is ready to be merged. The only thing remaining, if you don't find anything else is to double check the name of the artifacts.

sirocchj

Added a few more comments. @barambani wdyt?

README.md

sirocchj · 2021-01-02T14:13:17Z

build.sbt

@@ -135,15 +136,15 @@ lazy val commonSettings = Seq(
  licenses += "MIT" -> url("http://opensource.org/licenses/MIT"),
  developers += Developer("sirocchj", "Julien Sirocchi", "[email protected]", url("https://github.com/sirocchj")),
  scalacOptions ++= versionDependent(scalaVersion.value),
-  resolvers += "confluent" at "https://packages.confluent.io/maven/"
+  resolvers ++= Seq("confluent" at "https://packages.confluent.io/maven/", "snapshots" at "https://oss.sonatype.org/content/repositories/snapshots")


This we can probably revert too here

sirocchj · 2021-01-02T14:24:06Z

doobie/src/main/scala/tamer/db/Db.scala

+import java.time.Instant
+import scala.util.hashing.byteswap64
+
+object Db {


Do we need these classes wrapped in this Db object for some reason?

Co-authored-by: Julien Jean Paul Sirocchi <[email protected]>

barambani · 2020-12-29T14:16:08Z

build.sbt

@@ -19,6 +19,7 @@ lazy val V = new {
  val scalatest     = "3.2.3"
  val silencer      = "1.7.1"
  val zio           = "1.0.3"
+  val `zio-s3`      = "latest.integration"


Suggested change

val `zio-s3` = "latest.integration"

val `zio-s3` = "0.2.5"

https://github.com/zio/zio-s3/releases/tag/v0.2.5

barambani · 2020-12-29T15:01:21Z

build.sbt

@@ -135,15 +138,15 @@ lazy val commonSettings = Seq(
  licenses += "MIT" -> url("http://opensource.org/licenses/MIT"),
  developers += Developer("sirocchj", "Julien Sirocchi", "[email protected]", url("https://github.com/sirocchj")),
  scalacOptions ++= versionDependent(scalaVersion.value),
-  resolvers += "confluent" at "https://packages.confluent.io/maven/"
+  resolvers ++= Seq("confluent" at "https://packages.confluent.io/maven/", "snapshots" at "https://oss.sonatype.org/content/repositories/snapshots")


which dependency needs snapshots ?

will remove this, already removed the zio-s3 dep

barambani · 2021-01-02T17:43:38Z

core/src/main/scala/tamer/kafka/Kafka.scala

+            ZStream
+              .fromEffect(subscribe(sc))
+              .flatMap {
+                case true => ZStream.fromEffect(log.info(s"consumer group ${cfg.state.groupId} resuming consumption from ${cfg.state.topic}"))


it seems about time to support Zio Streams better in log-effect

that sounds like interesting work

barambani · 2021-01-02T17:57:06Z

core/src/main/scala/tamer/kafka/Kafka.scala

@@ -20,7 +18,7 @@ import zio.kafka.consumer.{CommittableRecord, Consumer, ConsumerSettings, Subscr
 import zio.kafka.producer.{Producer, ProducerSettings}
 import zio.stream.ZStream

-final case class StateKey(queryHash: String, groupId: String)
+final case class StateKey(stateKey: String, groupId: String)

 object Kafka {
  trait Service {


Suggested change

trait Service {

sealed trait Service {

will do, should work fine, but I don't think it's idiomatic

barambani · 2021-01-02T18:05:12Z

core/src/main/scala/tamer/config/Config.scala

@@ -53,17 +41,12 @@ object Config {
    kafkaSinkConfigValue,
    kafkaStateConfigValue
  ).parMapN(Kafka)
-  private[this] val tamerConfigValue: ConfigValue[Tamer] = (dbConfigValue, queryConfigValue, kafkaConfigValue).parMapN(Tamer.apply)

  trait Service {


Who's using this ? is it needed ?

Probably hereditary of when this was a module, will remove

barambani · 2021-01-02T19:52:38Z

doobie/src/main/scala/tamer/db/package.scala

+    val kafkaLayer: Layer[TamerError, Kafka]                                      = Config.live >>> Kafka.live
+    val transactorLayer: Layer[TamerError, DbTransactor]                          = (Blocking.live ++ ConfigDb.live) >>> db.hikariLayer
+    val queryConfigLayer: Layer[TamerError, DbConfig with QueryConfig]            = ConfigDb.live
+    val defaultLayer: Layer[TamerError, DbTransactor with Kafka with QueryConfig] = transactorLayer ++ queryConfigLayer ++ kafkaLayer


these don't seem to depend on setup or any of the type parameters so they could live outside of fetch

barambani · 2021-01-02T19:57:14Z

doobie/src/main/scala/tamer/db/package.scala

+  type DbTransactor  = Has[Transactor[Task]]
+  type TamerDBConfig = DbTransactor with QueryConfig
+
+  implicit final class InstantOps(private val instant: Instant) extends AnyVal {


this doesn't look specific of the db flavour, it could be in core as an helper syntax.

But it's only used there and in relation to db examples. I would leave it here since it's an hapax legomenon.

barambani · 2021-01-02T20:12:19Z

doobie/src/main/scala/tamer/db/package.scala

+    mkSetup(queryBuilder)(timeSegment, keyExtract, stateFold)
+  }
+
+  final def mkSetup[


Considering we have a Setup case class, couldn't this be its apply ?

object Setup { final def apply[ K <: Product: Encoder: Decoder: SchemaFor, V <: Product: Encoder: Decoder: SchemaFor, S <: Product with HashableState: Encoder: Decoder: SchemaFor ]( queryBuilder: S => Query0[V] )(defaultState: S, keyExtract: V => K, stateFoldM: S => QueryResult[V] => UIO[S]): Setup[K, V, S] = [...]

also, mkSetupWithTimeSegment could be a smart constructor in the Setup companion.

Ah yeah, that would make ti more regular, will do.

barambani · 2021-01-02T20:24:42Z

doobie/src/main/scala/tamer/db/package.scala

+
+  private[this] val logTask: Task[LogWriter[Task]] = log4sFromName.provide("tamer.db")
+
+  final def iteration[K <: Product, V <: Product, S <: Product with HashableState](


is iteration expected to be part of the public api ? Considering we have fetch and fetchWithTimeSegment it could be private

no, let's make it private for now

barambani · 2021-01-02T20:26:16Z

doobie/src/main/scala/tamer/db/package.scala

+    } yield managedTransactor
+  }
+
+  def mkTransactor(db: ConfigDb.Db, connectEC: ExecutionContext, transactEC: ExecutionContext): Managed[TamerError, HikariTransactor[Task]] =


do we want to expose mkTransactor as part of the public api ?

I’m assuming not

barambani · 2021-01-02T20:29:42Z

This is good progress. It will be great to merge it in.

sirocchj · 2021-01-02T20:47:55Z

build.sbt

+  .dependsOn(tamer)
+  .settings(commonSettings)
+  .settings(
+    name := "doobie",


Yeah, we probably want to prefix all modules' artifacts with tamer- now that we no longer have only one. This means

Suggested change

name := "doobie",

name := "tamer-doobie",

And likely tamer-core the other one.

As opposed to just just tamer, because the module in itself doesn't do much without a supporting module to fetch data. Done.

gurghet · 2021-01-02T22:28:50Z

Ok, should be good.

sirocchj

One last pass then we'll be good to go

sirocchj · 2021-01-03T09:49:39Z

local/runDb.sh

+SCRIPT_PATH=$(cd "$(dirname "${BASH_SOURCE[0]}")" || exit; pwd -P)
+cd "$SCRIPT_PATH"/.. || exit
+
+sbt "example/runMain tamer.example.Main" -jvm-debug 5005


Suggested change

sbt "example/runMain tamer.example.Main" -jvm-debug 5005

sbt "example/runMain tamer.example.DatabaseSimple" -jvm-debug 5005

sirocchj · 2021-01-03T09:51:18Z

README.md

+
+### Database module
+
+Basic manual testing is available for the code in the example module `tamer.example.Main`.


Suggested change

Basic manual testing is available for the code in the example module `tamer.example.Main`.

Basic manual testing is available for the code in the example module `tamer.example.DatabaseSimple` (and/or `tamer.example.DatabaseGeneralized`).

sirocchj · 2021-01-03T09:51:51Z

README.md

+
+From the `local` folder launch `docker-compose up` (you can enter `docker-compose down`
+if you want to start from scratch). After that you should be able to access the kafka
+gui from http://localhost:8000.


Suggested change

gui from http://localhost:8000.

gui from [http://localhost:8000](http://localhost:8000).

sirocchj · 2021-01-03T09:59:39Z

core/src/main/scala/tamer/Setup.scala

+    val defaultState: S,
+    val stateKey: Int
+) {
+  def show: String = "not available, please implement the show method to display setup"


I agree with @barambani here, can we remove the default message and leave def show: String abstract or is there a reason we're missing?

sirocchj · 2021-01-03T10:03:56Z

core/src/main/scala/tamer/config/Config.scala

@@ -13,8 +13,6 @@ import zio.interop.catz._
 import scala.concurrent.duration.FiniteDuration

 object Config {
-  final case class Db(driver: NonEmptyString, uri: UriString, username: NonEmptyString, password: Password)


looks like there's two refinement types left in config's package object that should be moved to the doobie module in the db package object: UriString and Password

sirocchj · 2021-01-03T10:12:59Z

doobie/src/main/scala/tamer/db/ConfigDb.scala

+    val queryConfig: URIO[QueryConfig, Query]
+  }
+
+  val live: Layer[TamerError, Has[Db] with Has[Query]] = ZLayer.fromEffectMany {


Suggested change

val live: Layer[TamerError, Has[Db] with Has[Query]] = ZLayer.fromEffectMany {

val live: Layer[TamerError, DbConfig with QueryConfig] = ZLayer.fromEffectMany {

sirocchj · 2021-01-03T10:15:12Z

doobie/src/main/scala/tamer/db/ConfigDb.scala

+
+  trait Service {
+    val dbConfig: URIO[DbConfig, Db]
+    val queryConfig: URIO[QueryConfig, Query]
+  }


this can all go, right?

Suggested change

trait Service {

val dbConfig: URIO[DbConfig, Db]

val queryConfig: URIO[QueryConfig, Query]

}

Technically yes, but maybe it's not the best form, anyways we can refactor later to decide who deserves to be a service and who doesn't

sirocchj · 2021-01-03T10:17:20Z

doobie/src/main/scala/tamer/db/Setup.scala

@@ -0,0 +1,84 @@
+package tamer.db


Suggested change

package tamer.db

package tamer

package db

sirocchj · 2021-01-03T10:17:42Z

doobie/src/main/scala/tamer/db/Setup.scala

+
+import com.sksamuel.avro4s.{Decoder, Encoder, SchemaFor}
+import doobie.Query0
+import tamer.Serde


Suggested change

import tamer.Serde

sirocchj · 2021-01-03T10:19:06Z

doobie/src/main/scala/tamer/db/Setup.scala

+    override val defaultState: S,
+    keyExtract: V => K,
+    stateFoldM: S => QueryResult[V] => UIO[S]
+) extends tamer.Setup[K, V, S](


this may be required

Suggested change

) extends tamer.Setup[K, V, S](

) extends _root_.tamer.Setup[K, V, S](

gurghet · 2021-01-03T16:44:24Z

For some reason I can't answer to #212 (comment) but the reason is that it's an advanced feature and new implementors might want to defer it instead of simply implementing it as an empty string. So it's like I’m giving them a placeholder for free. Otherwise there are two possibilities:

They just implement it as soon as they write first version of their module, in this case abstract or not there is no difference
They leave it as a nice to have, in this case they are forced to hack it, which is not ideal

sirocchj

Nice work! And thank you! Hopefully this will be the basis for a good few source connectors!

barambani · 2021-01-03T22:02:29Z

For some reason I can't answer to #212 (comment) but the reason is that it's an advanced feature and new implementors might want to defer it instead of simply implementing it as an empty string. So it's like I’m giving them a placeholder for free. Otherwise there are two possibilities:
1. They just implement it as soon as they write first version of their module, in this case abstract or not there is no difference

2. They leave it as a nice to have, in this case they are forced to hack it, which is not ideal

in this spirit, does Setup need to be abstract ?

barambani · 2021-01-03T22:16:18Z

anyway the above is no reason to hold the pr. That's good to go. Brilliant work.

gurghet · 2021-01-03T22:25:05Z

For some reason I can't answer to #212 (comment) but the reason is that it's an advanced feature and new implementors might want to defer it instead of simply implementing it as an empty string. So it's like I’m giving them a placeholder for free. Otherwise there are two possibilities:
1. They just implement it as soon as they write first version of their module, in this case abstract or not there is no difference

2. They leave it as a nice to have, in this case they are forced to hack it, which is not ideal
in this spirit, does Setup need to be abstract ?

What do you have in mind? It certainly can't be concrete now that we separated it from the db module.

barambani · 2021-01-03T22:50:37Z

yes, something like a case class that can be used as it is for a default setup, and specialised in case the print (or future behaviours) need to be more specific.

UPDATE: anyway as said I'll wait when the public api is more mature and there are other examples of user code. I still don't see what make life easier or more complicated.

gurghet added 3 commits December 26, 2020 16:35

compiles

5704ca2

config extracted barely compiles

b981d3a

working (apparently)

8b5c151

gurghet requested review from barambani and sirocchj as code owners December 28, 2020 20:13

gurghet marked this pull request as draft December 28, 2020 20:13

Merge branch 'master' into front-end-rehaul

75d47b6

gurghet commented Dec 28, 2020

View reviewed changes

sirocchj reviewed Dec 28, 2020

View reviewed changes

gurghet added 3 commits December 29, 2020 23:11

feature parity

d99eefc

Merge branch 'front-end-rehaul' of github.com:gurghet/tamer into fron…

bc40298

…t-end-rehaul

address review

71bf64a

gurghet mentioned this pull request Dec 29, 2020

use nanoTime instead of wall clock time to guarantee monotonicity #213

Closed

gurghet and others added 7 commits December 30, 2020 00:58

just shove the layers under the carpet

2095253

reorder main

c6e4d6d

Datables don't need the user to define ordering anymore

d3e9874

dateable => timestamped and some scaladoc

e8f6b91

Update core/src/main/scala/tamer/Setup.scala

035745e

Co-authored-by: Julien Jean Paul Sirocchi <[email protected]>

address pr review

4674617

leave the stateKey as int

831def7

sirocchj previously approved these changes Dec 30, 2020

View reviewed changes

cleaned, update docs

6e6026c

gurghet dismissed sirocchj’s stale review via 6e6026c January 1, 2021 20:50

gurghet requested a review from sirocchj January 1, 2021 20:53

gurghet marked this pull request as ready for review January 1, 2021 20:57

sirocchj reviewed Jan 2, 2021

View reviewed changes

Update README.md

532c69b

Co-authored-by: Julien Jean Paul Sirocchi <[email protected]>

barambani reviewed Jan 2, 2021

View reviewed changes

sirocchj reviewed Jan 2, 2021

View reviewed changes

gurghet added 2 commits January 2, 2021 22:27

final draft after review

6578e7d

Merge branch remote branch

35b2fbe

sirocchj reviewed Jan 3, 2021

View reviewed changes

final final draft reviewed

c1097e0

gurghet force-pushed the front-end-rehaul branch from ba7ec60 to c1097e0 Compare January 3, 2021 17:27

sirocchj approved these changes Jan 3, 2021

View reviewed changes

barambani approved these changes Jan 3, 2021

View reviewed changes

sirocchj merged commit 7e037d9 into laserdisc-io:master Jan 3, 2021

gurghet deleted the front-end-rehaul branch January 3, 2021 22:24


		private[this] val logTask: Task[LogWriter[Task]] = log4sFromName.provide("tamer.db")

		final def iteration[K <: Product, V <: Product, S <: Product with HashableState](

	sbt "example/runMain tamer.example.Main" -jvm-debug 5005
	sbt "example/runMain tamer.example.DatabaseSimple" -jvm-debug 5005


		### Database module

		Basic manual testing is available for the code in the example module `tamer.example.Main`.

	gui from http://localhost:8000.
	gui from [http://localhost:8000](http://localhost:8000).

	val live: Layer[TamerError, Has[Db] with Has[Query]] = ZLayer.fromEffectMany {
	val live: Layer[TamerError, DbConfig with QueryConfig] = ZLayer.fromEffectMany {

	) extends tamer.Setup[K, V, S](
	) extends _root_.tamer.Setup[K, V, S](

Front end rehaul #212

Front end rehaul #212

Conversation

gurghet commented Dec 28, 2020 • edited Loading

gurghet Dec 28, 2020 • edited Loading

Choose a reason for hiding this comment

sirocchj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurghet Dec 28, 2020 • edited Loading

Choose a reason for hiding this comment

gurghet commented Dec 30, 2020

sirocchj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurghet commented Jan 1, 2021

sirocchj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barambani commented Jan 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurghet commented Jan 2, 2021

sirocchj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirocchj Jan 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirocchj Jan 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gurghet commented Jan 3, 2021 • edited Loading

sirocchj left a comment

Choose a reason for hiding this comment

barambani commented Jan 3, 2021

barambani commented Jan 3, 2021

gurghet commented Jan 3, 2021

barambani commented Jan 3, 2021 • edited Loading

gurghet commented Dec 28, 2020 •

edited

Loading

gurghet Dec 28, 2020 •

edited

Loading

gurghet Dec 28, 2020 •

edited

Loading

sirocchj Jan 3, 2021 •

edited

Loading

sirocchj Jan 3, 2021 •

edited

Loading

gurghet commented Jan 3, 2021 •

edited

Loading

barambani commented Jan 3, 2021 •

edited

Loading