Skip to content
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

Merged
merged 1 commit into from
Jun 8, 2018

Conversation

jklukas
Copy link
Contributor

@jklukas jklukas commented May 31, 2018

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.

@@ -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",
Copy link
Contributor Author

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",
Copy link
Contributor Author

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",
Copy link
Contributor Author

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"
Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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?

@jklukas jklukas requested a review from fbertsch May 31, 2018 15:15
@jklukas
Copy link
Contributor Author

jklukas commented May 31, 2018

Ran main_summary on nightly data using the master branch and this branch. The DatasetComparator confirmed that the output datasets were the same.

@jklukas
Copy link
Contributor Author

jklukas commented May 31, 2018

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.

Copy link
Contributor

@fbertsch fbertsch left a 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"
Copy link
Contributor

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",
Copy link
Contributor Author

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",
Copy link
Contributor Author

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
Copy link
Contributor Author

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"
Copy link
Contributor Author

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-io
Copy link

codecov-io commented Jun 6, 2018

Codecov Report

Merging #434 into master will decrease coverage by 0.69%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
...elemetry/experiments/analyzers/CrashAnalyzer.scala 26.31% <0%> (-73.69%) ⬇️

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 34b01a2...208c1dc. Read the comment docs.

@jklukas
Copy link
Contributor Author

jklukas commented Jun 8, 2018

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.

@jklukas
Copy link
Contributor Author

jklukas commented Jun 8, 2018

This is passing again with java.policy taken out, so ready for review again, @fbertsch

Copy link
Contributor

@fbertsch fbertsch left a 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.

@jklukas jklukas merged commit b5460d0 into mozilla:master Jun 8, 2018
@jklukas jklukas deleted the spark-2.3.0 branch June 8, 2018 18:08
jklukas added a commit that referenced this pull request Jun 14, 2018
#434
updated versions that changed the interface for Avro and caused
Longitudinal to fail.
jklukas added a commit that referenced this pull request Jun 15, 2018
#434
updated versions that changed the interface for Avro and caused
Longitudinal to fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants