-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spark to 2.3.0 and update other dependencies #434
Conversation
@@ -18,28 +20,29 @@ lazy val root = (project in file(".")). | |||
scalaModuleInfo := scalaModuleInfo.value.map(_.withOverrideScalaVersion(true)), | |||
libraryDependencies += "com.mozilla.telemetry" %% "moztelemetry" % "1.1-SNAPSHOT", | |||
libraryDependencies += "com.mozilla.telemetry" %% "spark-hyperloglog" % "2.0.0-SNAPSHOT", | |||
libraryDependencies += "org.apache.avro" % "avro" % "1.7.7", | |||
libraryDependencies += "org.apache.parquet" % "parquet-avro" % "1.7.0", | |||
libraryDependencies += "org.scalatest" %% "scalatest" % "3.0.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved all test dependencies to the bottom of the list and ensured they're only included in Test scope.
libraryDependencies += "joda-time" % "joda-time" % "2.9.2", | ||
libraryDependencies += "org.apache.hadoop" % "hadoop-client" % "2.7.1" excludeAll(ExclusionRule(organization = "javax.servlet")), | ||
libraryDependencies += "org.apache.hadoop" % "hadoop-aws" % "2.7.1" excludeAll(ExclusionRule(organization = "javax.servlet")), | ||
libraryDependencies += "com.github.nscala-time" %% "nscala-time" % "2.10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears we don't use nscala-time anymore, so removed it.
build.sbt
Outdated
libraryDependencies += "org.yaml" % "snakeyaml" % "1.21", | ||
libraryDependencies += "com.google.protobuf" % "protobuf-java" % "2.5.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't directly use this dependency, so removing it from the list here and instead just relying on the override.
run-sbt.sh
Outdated
@@ -12,15 +12,18 @@ if [ ! -f /.dockerenv ]; then | |||
exit $? | |||
fi | |||
|
|||
# Set options that sbt will pass to the JVM | |||
export SBT_OPTS="-Xss2M -Djava.security.policy=java.policy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The security policy is necessary due to a change in derby as discussed in #426
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to put a link here for reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a link in the java.policy file itself, and explanation of why it exists. Does that seem sufficient?
Ran main_summary on nightly data using the master branch and this branch. The DatasetComparator confirmed that the output datasets were the same. |
Well, I tested this and then changed just a few little things. Tests are now failing, so this will likely need to wait for next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, ping me when the tests are ready and I'll take another look.
run-sbt.sh
Outdated
@@ -12,15 +12,18 @@ if [ ! -f /.dockerenv ]; then | |||
exit $? | |||
fi | |||
|
|||
# Set options that sbt will pass to the JVM | |||
export SBT_OPTS="-Xss2M -Djava.security.policy=java.policy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to put a link here for reference.
libraryDependencies += "com.mozilla.telemetry" %% "moztelemetry" % "1.1-SNAPSHOT", | ||
libraryDependencies += "com.mozilla.telemetry" %% "spark-hyperloglog" % "2.0.0-SNAPSHOT", | ||
libraryDependencies += "org.apache.avro" % "avro" % "1.7.7", | ||
libraryDependencies += "org.apache.parquet" % "parquet-avro" % "1.7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These dependencies have moved further down the list and have updated versions
libraryDependencies += "net.sandrogrzicic" %% "scalabuff-runtime" % "1.4.0", | ||
libraryDependencies += "com.holdenkarau" %% "spark-testing-base" % "2.0.0_0.4.7" % "test", | ||
libraryDependencies += "org.xerial.snappy" % "snappy-java" % "1.1.2", | ||
libraryDependencies += "org.json4s" %% "json4s-jackson" % "3.2.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the json4s-jackson so that we get the version spark-core wants transitively.
@@ -53,7 +63,7 @@ test in assembly := {} | |||
|
|||
assemblyMergeStrategy in assembly := { | |||
case PathList("META-INF", xs @ _*) => MergeStrategy.discard | |||
case x => MergeStrategy.first | |||
case _ => MergeStrategy.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated style fixup. Using underscore to indicate that we don't care about the value in this case.
run-sbt.sh
Outdated
@@ -12,15 +12,18 @@ if [ ! -f /.dockerenv ]; then | |||
exit $? | |||
fi | |||
|
|||
# Set options that sbt will pass to the JVM | |||
export SBT_OPTS="-Xss2M -Djava.security.policy=java.policy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a link in the java.policy file itself, and explanation of why it exists. Does that seem sufficient?
Codecov Report
@@ Coverage Diff @@
## master #434 +/- ##
=========================================
- Coverage 65.01% 64.31% -0.7%
=========================================
Files 45 45
Lines 4007 4007
Branches 135 135
=========================================
- Hits 2605 2577 -28
- Misses 1402 1430 +28
Continue to review full report at Codecov.
|
Removed the java.policy reference since I'm no longer seeing exceptions thrown without the policy, so this PR is back to affecting only build.sbt. Squashed and rebased. If tests run successfully, this should be good to go. |
This is passing again with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, this looks good.
#434 updated versions that changed the interface for Avro and caused Longitudinal to fail.
#434 updated versions that changed the interface for Avro and caused Longitudinal to fail.
This brings back in the Spark version update merged in #426 and subsequently backed out.
It also updates several other libraries to more recent (most recent where possible) versions.