Remote scheme provider #53

fedefernandez · 2017-08-24T08:46:13Z

Fixes #51

This PR:

Adds the file MetadataSchemaProvider.scala implementing a SchemaProvider that fetches the information from a Cluster connection.
Includes a SchemaConversions trait for converting classes from Datastax to Troy
Removes the hardcoded test data in favour of scalacheck arbitrary instances.

codecov-io · 2017-08-24T08:50:20Z

Codecov Report

Merging #53 into master will decrease coverage by 0.22%.
The diff coverage is 88.97%.

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
- Coverage   88.77%   88.55%   -0.23%     
==========================================
  Files          15       17       +2     
  Lines         303      428     +125     
  Branches        0        2       +2     
==========================================
+ Hits          269      379     +110     
- Misses         34       49      +15

Impacted Files	Coverage Δ
core/src/main/scala/schema/package.scala	`100% <100%> (ø)`	⬆️
...scala/schema/provider/MetadataSchemaProvider.scala	`75% <75%> (ø)`
...a/schema/provider/metadata/SchemaConversions.scala	`91.39% <91.39%> (ø)`
...ain/scala/schema/provider/TroySchemaProvider.scala	`92.3% <92.3%> (+8.97%)`	⬆️
core/src/main/scala/api/package.scala	`100% <0%> (+100%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cfe86e...fa33b90. Read the comment docs.

peterneyens

I can't really comment about the Cassandra part, I only looked at the code itself.

I left a few suggestions and there is one place that would be nice if it could be improved, but it seems difficult without making things more complex.

peterneyens · 2017-08-24T09:34:46Z

core/src/main/scala/schema/provider/metadata/SchemaConversions.scala

+              for {
+                dataNative1 <- toDataTypeNative(tupleArgs._1)
+                dataNative2 <- toDataTypeNative(tupleArgs._2)
+              } yield DataType.Map(dataNative1, dataNative2)


Maybe :

for { t1 <- typeArgs.headOption t2 <- typeArgs.tail.headOption } yield (toDataTypeNative(t1) |@| toDataTypeNative(t2)).map(DataType.Map)

peterneyens · 2017-08-24T09:45:40Z

core/src/main/scala/schema/provider/MetadataSchemaProvider.scala

+        _        <- clusterAPI.connect
+        metadata <- clusterAPI.metadata
+        _        <- clusterAPI.close
+      } yield metadata


You could write this as

clusterAPI.connect >> clusterAPI.metadata << clusterAPI.close or

clusterAPI.connect *> clusterAPI.metadata <* clusterAPI.close.

Where the first one is a bit closer to what you wrote (as it is defined in flatMap syntax) while the second one (defined in cartesian syntax) is probably used more often. In this case there is no real difference.

Cool, I have a doubt here. I was trying to add a recover in the clusterAPI.metadata call, because we want to call to clusterAPI.close even when the previous call fails. What's the best way to achieve this from your point of view? Thanks

There is this open PR in Cats : typelevel/cats#1662 (issue typelevel/cats#1630).

Doobie uses something similar in its Transactor. ConnectionIO can contain side effects though (and can thus define a MonadError instance), and that is not something you can do with every Free / FreeS.

You can use the guarantee implementation from the PR linked to above after you interpret to an M with a MonadError[M, Throwable]
It is possible we can do something like they do in quasar to create a MonadError for FreeS if your algebras contain the Error effect, but that would need some more thought / work.

peterneyens · 2017-08-24T10:10:01Z

core/src/main/scala/schema/provider/MetadataSchemaProvider.scala

+        tables    <- tableList.traverse(toCreateTable)
+        indexes   <- indexList.traverse(toCreateIndex(_))
+        userTypes <- userTypeList.traverse(toUserType)
+      } yield keyspaces ++ tables ++ indexes ++ userTypes


You could also use |+| here (Either s Semigroup uses the Semigroup of the right side, and for Semigroup[List].combine is ::: / ++).

keyspaceList.traverse(toCreateKeyspace) |+| tableList.traverse(toCreateTable) |+| indexList.traverse(toCreateIndex(_)) |+| userTypeList.traverse(toUserType)

peterneyens · 2017-08-24T10:19:44Z

core/src/main/scala/schema/provider/MetadataSchemaProvider.scala

+      } yield metadata
+
+    Either.catchNonFatal {
+      Await.result(metadataF[ClusterAPI.Op].interpret[Future], 10.seconds)


I can see that schemaDefinition needs to return an Either[...] at the moment, I am not sure if we can change that somehow.

Currently it is unfortunate twice: first we immediately have to interpret a FreeS and then we we have to block on a Future. It doesn't appear to be trivial to change this though ...

If there is no way we can do that, I would put the Await on the end.

val fut: Future[SchemaDefinitionProviderError, SchemaDefinition] = metadataF[ClusterAPI.Op].interpret[Future].attempt.map { _.leftMap (SchemaDefinitionProviderError(_)) flatMap { metadata => // ... } } Await.result(fut, 10.seconds) // :(

You're right, clearly the worst part of the PR :(

I created a ticket for changing the return type to M[_] (#52)

peterneyens · 2017-08-24T10:29:14Z

core/src/main/scala/schema/package.scala

+    def apply(e: Throwable): SchemaDefinitionProviderError =
+      SchemaDefinitionProviderError(e.getMessage, Some(e))
+  }
+


Maybe you could add a type alias for Either[SchemaDefinitionProviderError, ?] here?

raulraja · 2017-08-25T01:24:38Z

core/src/main/scala/schema/provider/MetadataSchemaProvider.scala

+            userTypeList.traverse(toUserType)
+        }
+      }
+    Await.result(fut, 10.seconds)


Is there a need to block if instead of all the methods in this class return a M[Whatever] where M[_] : AsyncContext and in this case is M[SchemaResult[SchemaDefinition]]? all the calls such as extractIndexes(tableList) etc.. would be part of a monadic chain instead of direct style.

I created ticket #52
Should I solve this on this PR? I'm ok with that, I was trying to keep this PR shorter as possible

In a new ticket is fine 👍

raulraja · 2017-08-25T01:26:33Z

core/src/main/scala/schema/provider/metadata/SchemaConversions.scala

+  def toCreateKeyspace(keyspaceMetadata: KeyspaceMetadata): SchemaResult[CreateKeyspace] =
+    Either.catchNonFatal {
+      val name: String = Option(keyspaceMetadata.getName)
+        .getOrElse(throw new NullPointerException("Schema name is null"))


Probably better to use an IllegalArgumentException in this case

raulraja · 2017-08-25T01:29:21Z

core/src/main/scala/schema/provider/metadata/SchemaConversions.scala

+
+    def toDataTypeNative(dataType: DatastaxDataType): SchemaResult[DataType.Native] =
+      dataType.getName match {
+        case Name.ASCII     => DataType.Ascii.asRight


Where is asRight defined? as suually denotes an internal cast and if this is is just lifting the value into Either.Right we should use toRight or the right() syntax from cats.

asRight is the new syntax in Cats (see PR typelevel/cats#1454).

asRight and asLeft where chosen so they can't be confused with the .right and .left methods on Either for RightProjection and LeftProjection.

Fede Fernández added 6 commits August 16, 2017 08:02

Allows loading schema from files

3013a57

Adds a remote schema provider based on cluster metadata

27beabe

Adds tests and improves schema provider

5f9c81a

Removes hardcoded schema data and improves tests

107daaf

Adds the metadata schema provider tests

7283bd3

Moves reserved keywords to TestUtils

9c95653

fedefernandez added the in progress label Aug 24, 2017

fedefernandez requested a review from raulraja August 24, 2017 08:46

fedefernandez requested a review from peterneyens August 24, 2017 09:18

peterneyens reviewed Aug 24, 2017

View reviewed changes

Address PR comments

4d1ff8a

raulraja reviewed Aug 25, 2017

View reviewed changes

raulraja approved these changes Aug 29, 2017

View reviewed changes

Changes to IllegalArgumentException a NullPointerException

fa33b90

fedefernandez merged commit 76c52cb into master Aug 29, 2017

fedefernandez deleted the ff-issue-51-remote-scheme-provider branch August 29, 2017 09:25

fedefernandez removed the in progress label Aug 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote scheme provider #53

Remote scheme provider #53

fedefernandez commented Aug 24, 2017

codecov-io commented Aug 24, 2017 •

edited

Loading

peterneyens left a comment

peterneyens Aug 24, 2017

peterneyens Aug 24, 2017

fedefernandez Aug 24, 2017

peterneyens Aug 24, 2017

peterneyens Aug 24, 2017

peterneyens Aug 24, 2017

fedefernandez Aug 24, 2017

peterneyens Aug 24, 2017

fedefernandez Aug 24, 2017

raulraja Aug 25, 2017

fedefernandez Aug 25, 2017

raulraja Aug 29, 2017

raulraja Aug 25, 2017

raulraja Aug 25, 2017

peterneyens Aug 25, 2017

Remote scheme provider #53

Remote scheme provider #53

Conversation

fedefernandez commented Aug 24, 2017

codecov-io commented Aug 24, 2017 • edited Loading

Codecov Report

peterneyens 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

codecov-io commented Aug 24, 2017 •

edited

Loading