Skip to content
This repository was archived by the owner on Feb 14, 2025. It is now read-only.

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.

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.

libraryDependencies += "org.scalaj" %% "scalaj-http" % "2.3.0",
libraryDependencies += "org.scalaj" %% "scalaj-http" % "2.4.0",
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
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
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.

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
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 jklukas requested a review from fbertsch June 6, 2018 18:24
@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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants