Preview of upgrade to Scala 2.12. #342

ghost · 2017-02-21T16:58:05Z

Incremented all required versions to value that included scala 2.12 support. See diff for details.
Modified RedisStore to use Buf instead of ChannelBuffer due to changes in underlying client. To ease this transition a method to convert Store[Buf, Buf] to Store[ChannelBuffer, ChannelBuffer] is provided on RedisStore.
Removed support for Scala 2.10. Two libraries lacked support for scala 2.10 and 2.12 on the same version: twiiter-util and finagle. Supporting 2.10 finagle and 2.12 finagle was not possible due to interface changes.
Various packages changed and were fixed.
Added target to travis.yml and build.sbt to include scala 2.12.
Set source and build target to java 8 to support scala 2.12.

ghost · 2017-02-21T16:59:55Z

I had some trouble getting the tests to run consistently on travis.ci. When I ran the tests locally against each data-store running in docker everything worked.

Noticed the mongo-db tests create ~10k connections during the test run this seemed to be part of why travis has trouble with the build.

BenFradet · 2017-02-21T17:04:02Z

Nice, this would solve #310 as well 👍

ghost · 2017-02-21T17:06:51Z

build.sbt

@@ -227,9 +230,8 @@ lazy val storehausMemcache = module("memcache").settings(
    "com.twitter" %% "bijection-core" % bijectionVersion,
    "com.twitter" %% "bijection-netty" % bijectionVersion,
    "com.twitter" %% "finagle-memcached" % finagleVersion excludeAll(
-      // we don't use this and its not on maven central.
-      ExclusionRule("com.twitter.common.zookeeper"),
-      ExclusionRule("com.twitter.common")


Part of the upgraded finagle memcache client needed com.twitter.common.metrics.Gauge so this exclude was removed.

johnynek · 2017-02-21T17:16:22Z

build.sbt

-val algebirdVersion = "0.12.0"
-val bijectionVersion = "0.9.1"
-val utilVersion = "6.34.0"
+val algebirdVersion = "0.12.4"


this should be 0.13.0

johnynek · 2017-02-21T17:17:20Z

build.sbt

@@ -322,7 +324,7 @@ lazy val storehausTesting = Project(
  settings = sharedSettings ++ Seq(
    name := "storehaus-testing",
    libraryDependencies ++= Seq(
-      "org.scalacheck" %% "scalacheck" % "1.12.2" withSources(),
+      "org.scalacheck" %% "scalacheck" % "1.12.6" withSources(),


can we go to 1.13 branch? Also, while we are at it, can we move this version up to the top of the file where we set the other versions?

ghost · 2017-02-21T17:26:52Z

@johnynek is the general preference to be on the max version of each lib? I set the versions to the min available.

johnynek · 2017-02-21T17:28:55Z

The general preference is to be on the minimum version that gives us what we need, that is also binary compatible with the latest version. Consumers of this library can then set.

For scalacheck, 1.13 has some significantly better features (better Function generation, and I thought was needed for 2.12).

ghost · 2017-02-21T17:33:20Z

Great. Thanks for that guidance. I will go back over the versions set here to make sure everything is aligned with that.

ghost · 2017-02-21T17:35:53Z

Looking at the travis ci failure it might be that the level of concurrency is too high for mysql and the other stores to handle during these property tests.

piyushnarang · 2017-04-11T23:29:33Z

@ben-adazza - Published the 2.12 friendly version of Scalding - 0.17.0. Can you try and pick that up in your PR?

piyushnarang · 2017-04-11T23:36:01Z

build.sbt

+val utilVersion = "6.39.0"
+
+//Scalding is not yet on  scala 12 so this needs to be
+// rev'ed to get the PR to compile for 12.
 val scaldingVersion = "0.16.0-RC1"


can upgrade to 0.17.0

Excellent. I will do that right away. Thanks!

1. Incremented all required versions to value that included scala 2.12 support. See diff for details. 2. Modified RedisStore to use Buf instead of ChannelBuffer due to changes in underlying client. To ease this transition a method to convert Store[Buf, Buf] to Store[ChannelBuffer, ChannelBuffer] is provided on RedisStore. 3. Removed support for Scala 2.10. Two libraries lacked support for scala 2.10 and 2.12 on the same version: twiiter-util and finagle. Supporting 2.10 finagle and 2.12 finagle was not possible due to interface changes. 4. Various packages changed and were fixed. 5. Added target to travis.yml and build.sbt to include scala 2.12. 6. Set source and build target to java 8 to support scala 2.12.

piyushnarang · 2017-04-14T17:28:52Z

.travis.yml

      script: umask 0022 && ./sbt ++$TRAVIS_SCALA_VERSION clean coverage test coverageReport mimaReportBinaryIssues
      after_success:
        - bash <(curl -s https://codecov.io/bash)
+#    - scala: 2.12.1


we can uncomment this now right?

I think we dont need to run coverage (as we're doing this for 2.11) and mima (as this will be the first 2.12 build).

piyushnarang · 2017-04-14T17:36:15Z

Took a quick look and it seems like a bunch of our mysql tests are failing due to:

WARNING: Exception propagated to the default monitor (upstream address: n/a, downstream address: /127.0.0.1:3306, label: 127.0.0.1:3306).
com.twitter.finagle.mysql.ServerError: Too many connections
	at com.twitter.finagle.mysql.ClientDispatcher$$anonfun$com$twitter$finagle$mysql$ClientDispatcher$$decodePacket$6.apply(ClientDispatcher.scala:228)
	at com.twitter.finagle.mysql.ClientDispatcher$$anonfun$com$twitter$finagle$mysql$ClientDispatcher$$decodePacket$6.apply(ClientDispatcher.scala:226)
	at com.twitter.util.Future$$anonfun$flatMap$1.apply(Future.scala:1089)
	at com.twitter.util.Future$$anonfun$flatMap$1.apply(Future.scala:1088)
...

ghost · 2017-04-14T17:40:08Z

Yeah. I think the properties tests create 10s of thousands on of connections since they throw away an entire client after each test.

piyushnarang · 2017-04-14T17:54:00Z

@ben-adazza yeah we either need to update them to close the connection or set the mysql connection limits higher - https://dev.mysql.com/doc/refman/5.7/en/too-many-connections.html
Curious as to why this was not failing earlier

ghost · 2017-04-14T22:44:21Z

Likely a change to the underlying implementation in finagle-mysql.

piyushnarang · 2017-04-14T23:43:19Z

build.sbt

+val utilVersion = "6.39.0"
+
+val scaldingVersion = "0.17.0"
+val finagleVersion = "6.41.0"


think the latest version is 6.43.0. Let's use that?

piyushnarang · 2017-04-15T00:45:21Z

@ben-adazza so looking at the example listed in the finagle-mysql repo: https://github.com/twitter/finagle/blob/master/finagle-example/src/main/scala/com/twitter/finagle/example/mysql/Example.scala#L45
And then looking at our code, https://github.com/twitter/storehaus/blob/develop/storehaus-mysql/src/test/scala/com/twitter/storehaus/mysql/MySqlStoreProperties.scala#L141 it seems as though we're setting things up to call close. I tried repro-ing this on my machine but my mysql setup is horked. Are you able to repro and see if the close is being called / not?

pankajroark · 2017-05-04T23:21:17Z

I'm able to repro the mysql test failure on my machine. Something that worked for me is sharing a client across tests. But client is not thread safe, so I also had to put the invocations in a synchronized block. Here's the patch:

diff --git a/storehaus-mysql/src/test/scala/com/twitter/storehaus/mysql/MySqlLongStoreProperties.scala b/storehaus-mysql/src/test/scala/com/twitter/storehaus/mysql/MySqlLongStoreProperties.scala
index 378e8bb..c2ab74b 100644
--- a/storehaus-mysql/src/test/scala/com/twitter/storehaus/mysql/MySqlLongStoreProperties.scala
+++ b/storehaus-mysql/src/test/scala/com/twitter/storehaus/mysql/MySqlLongStoreProperties.scala
@@ -89,6 +89,12 @@ object MySqlLongStoreProperties extends Properties("MySqlLongStore")
     isMatch
   }
 
+  // these should match mysql setup used in .travis.yml
+  lazy val client = Mysql.client
+    .withCredentials("storehaususer", "test1234")
+    .withDatabase("storehaus_test")
+    .newRichClient("127.0.0.1:3306")
+
   property("MySqlLongStore put and get") =
     withStore(putAndGetStoreTest(_), "text", "bigint")
 
@@ -97,18 +103,15 @@ object MySqlLongStoreProperties extends Properties("MySqlLongStore")
 
   private def withStore[T](f: MySqlLongStore => T, kColType: String, vColType: String,
       merge: Boolean = false): T = {
-    val client = Mysql.client
-      .withCredentials("storehaususer", "test1234")
-      .withDatabase("storehaus_test")
-      .newRichClient("127.0.0.1:3306")
-    // these should match mysql setup used in .travis.yml
 
     val tableName = s"storehaus-mysql-long-$kColType-$vColType${if (merge) "-merge" else ""}"
     val schema = s"CREATE TEMPORARY TABLE IF NOT EXISTS `$tableName` (`key` $kColType DEFAULT " +
       s"NULL, `value` $vColType DEFAULT NULL) ENGINE=InnoDB DEFAULT CHARSET=utf8;"
-    Await.result(client.query(schema))
+    this.synchronized {
+      Await.result(client.query(schema))
+      f(newStore(client, tableName))
+    }
 
-    f(newStore(client, tableName))
   }
 
   def newStore(client: Client, tableName: String): MySqlLongStore =
diff --git a/storehaus-mysql/src/test/scala/com/twitter/storehaus/mysql/MySqlStoreProperties.scala b/storehaus-mysql/src/test/scala/com/twitter/storehaus/mysql/MySqlStoreProperties.scala
index 26633ff..c192bc8 100644
--- a/storehaus-mysql/src/test/scala/com/twitter/storehaus/mysql/MySqlStoreProperties.scala
+++ b/storehaus-mysql/src/test/scala/com/twitter/storehaus/mysql/MySqlStoreProperties.scala
@@ -99,6 +99,12 @@ object MySqlStoreProperties extends Properties("MySqlStore")
       s"expected value $expected, but found $found")
   }
 
+  // these should match mysql setup used in .travis.yml
+  lazy val client = Mysql.client
+    .withCredentials("storehaususer", "test1234")
+    .withDatabase("storehaus_test")
+    .newRichClient("127.0.0.1:3306")
+
   property("MySqlStore text->text") =
     withStore(putAndGetStoreTest(_), "text", "text")
 
@@ -140,17 +146,14 @@ object MySqlStoreProperties extends Properties("MySqlStore")
 
   private def withStore[T](f: MySqlStore => T, kColType: String, vColType: String,
       multiGet: Boolean = false): T = {
-    val client = Mysql.client
-      .withCredentials("storehaususer", "test1234")
-      .withDatabase("storehaus_test")
-      .newRichClient("127.0.0.1:3306")
-    // these should match mysql setup used in .travis.yml
 
     val tableName = s"storehaus-mysql-$kColType-$vColType${if (multiGet) "-multiget" else ""}"
     val schema = s"CREATE TEMPORARY TABLE IF NOT EXISTS `$tableName` (`key` $kColType " +
       s"DEFAULT NULL, `value` $vColType DEFAULT NULL) ENGINE=InnoDB DEFAULT CHARSET=utf8;"
-    Await.result(client.query(schema))
-    f(newStore(client, tableName))
+    this.synchronized {
+      Await.result(client.query(schema))
+      f(newStore(client, tableName))
+    }
   }
 
   def newStore(client: Client, tableName: String): MySqlStore =

pankajroark · 2017-05-04T23:25:00Z

@ben-adazza btw are you still interested in pursuing this PR? I need to upgrade storehaus to scala 2.12 urgently, so I'm happy to take over this work in another PR if needed. Of course really appreciate the work you've already done. Happy to help in any way to get this upgrade shipped.

ghost · 2017-05-05T00:03:53Z

@pankajroark by all means you can take this code and get the tests running. Debugging these tests is not high on my priority list right now.

pankajroark · 2017-05-05T18:13:29Z

Fyi: the thing that directly breaks the mysql test is scalacheck update. But the fundamental problem is that the test doesn't close the client. Client can be closed only after the property is used in the tests. I'm trying to find a hook that executes after a property is done testing, to put the client close code there.

ghost · 2017-07-21T02:29:54Z

This work was very kindly taken over in: https://github.com/twitter/storehaus/pull/346/files

ghost changed the title ~~Preview of upgrades to Scala 2.12.~~ Preview of upgrade to Scala 2.12. Feb 21, 2017

ghost commented Feb 21, 2017

View reviewed changes

johnynek reviewed Feb 21, 2017

View reviewed changes

johnynek mentioned this pull request Mar 16, 2017

Upgrate to Finagle 6.43.0 and preparing the path for Scala 2.12 #344

Open

piyushnarang reviewed Apr 11, 2017

View reviewed changes

piyushnarang reviewed Apr 14, 2017

View reviewed changes

pankajroark mentioned this pull request May 7, 2017

Scala 2.12 upgrade preview with mysql tests fixed #346

Merged

ghost closed this Jul 21, 2017

ghost deleted the real-upgrade-branch branch July 21, 2017 02:29

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preview of upgrade to Scala 2.12. #342

Preview of upgrade to Scala 2.12. #342

ghost commented Feb 21, 2017 •

edited by ghost

Loading

ghost commented Feb 21, 2017

BenFradet commented Feb 21, 2017

ghost Feb 21, 2017

johnynek Feb 21, 2017

johnynek Feb 21, 2017

ghost commented Feb 21, 2017

johnynek commented Feb 21, 2017

ghost commented Feb 21, 2017

ghost commented Feb 21, 2017

piyushnarang commented Apr 11, 2017

piyushnarang Apr 11, 2017

ghost Apr 11, 2017

piyushnarang Apr 14, 2017

piyushnarang Apr 14, 2017

piyushnarang commented Apr 14, 2017

ghost commented Apr 14, 2017

piyushnarang commented Apr 14, 2017

ghost commented Apr 14, 2017

piyushnarang Apr 14, 2017

piyushnarang commented Apr 15, 2017

pankajroark commented May 4, 2017 •

edited

Loading

pankajroark commented May 4, 2017

ghost commented May 5, 2017

pankajroark commented May 5, 2017

ghost commented Jul 21, 2017

Preview of upgrade to Scala 2.12. #342

Preview of upgrade to Scala 2.12. #342

Conversation

ghost commented Feb 21, 2017 • edited by ghost Loading

ghost commented Feb 21, 2017

BenFradet commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 21, 2017

johnynek commented Feb 21, 2017

ghost commented Feb 21, 2017

ghost commented Feb 21, 2017

piyushnarang commented Apr 11, 2017

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

piyushnarang commented Apr 14, 2017

ghost commented Apr 14, 2017

piyushnarang commented Apr 14, 2017

ghost commented Apr 14, 2017

Choose a reason for hiding this comment

piyushnarang commented Apr 15, 2017

pankajroark commented May 4, 2017 • edited Loading

pankajroark commented May 4, 2017

ghost commented May 5, 2017

pankajroark commented May 5, 2017

ghost commented Jul 21, 2017

ghost commented Feb 21, 2017 •

edited by ghost

Loading

pankajroark commented May 4, 2017 •

edited

Loading