-
Notifications
You must be signed in to change notification settings - Fork 54
add flogger backend dependency #517
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
add flogger backend dependency #517
Conversation
64ab401
to
c1594b5
Compare
Codecov Report
@@ Coverage Diff @@
## master #517 +/- ##
=======================================
Coverage 69.63% 69.63%
=======================================
Files 36 36
Lines 1541 1541
Branches 133 133
=======================================
Hits 1073 1073
Misses 468 468
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
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.
fine to merge this to unblock for now but the fact that I don't understand what is going on worries me
build.sbt
Outdated
@@ -180,7 +181,8 @@ lazy val ratatoolSampling = project | |||
"com.twitter" %% "algebird-core" % algebirdVersion, | |||
"joda-time" % "joda-time" % jodaTimeVersion, | |||
"org.scalacheck" %% "scalacheck" % scalaCheckVersion, | |||
"org.scalatest" %% "scalatest" % scalaTestVersion % "test" | |||
"org.scalatest" %% "scalatest" % scalaTestVersion % "test", | |||
"com.google.flogger" % "flogger-system-backend" % floggerVersion % "test" |
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.
If the issues occurs in ParquetIOTest can we move this dep to ratatool-extras
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 docs for flogger say that "the dependency on flogger-system-backend is only required to be included when the binary is run", so I think there is a transitive dependency that is using flogger, but is excluding flogger-backend. Since we aren't supplying a logging backend, flogger is defaulting to throwing an exception.
The strange thing is that you weren't able to replicate it and the tests pass on CI.
Yea that's what I'm referring to. My gut says there's a better way to fix this than adding this dep |
It looks like these tests call |
45baa4c
to
c265521
Compare
I was getting a
error when running the tests locally in ratatool. It looks like we need to add a logging backend to ratatool in order to have flogger function properly. According to the flogger docs, https://github.com/google/flogger#1-add-the-dependencies-on-flogger, we need to add https://mvnrepository.com/artifact/com.google.flogger/flogger-system-backend/0.7.4 to build.sbt.
I set the scope to
test
in order to avoid adding dependencies for our users binary.