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

LIVY-313. Fixed SparkRInterpreter always returning success. #307

Merged
merged 6 commits into from
Mar 23, 2017

Conversation

jonalter
Copy link
Contributor

@jonalter jonalter commented Mar 9, 2017

  • Stopped redirecting stderr to stdout.
  • Continue to read ErrorStream (it was only being read once).
  • Checking for any errors returned by stderr before returning success.

LIVY-313

- Stopped redirecting stderr to stdout.
- Continue to read ErrorStream (it was only being read once).
- Checking for any errors returned by stderr before returning success.
@alex-the-man
Copy link
Contributor

Mind if you fix the style issue?

@alex-the-man
Copy link
Contributor

There used to be a race condition in the code I fixed in #251. To solve it, I modified the code to redirect stderr to stdout so SparkRInterpreter doesn't need to read from multiple streams. To repo the issue, run this unit test in a loop.

I think this fix undoes my fix.

@jonalter
Copy link
Contributor Author

@alex-the-man thanks for the feedback. Could you tell me how you run a single test or single test suite? I have been doing it by building my own class path and running org.scalatest.tools.Runner manually. I imagine there has to be an easier way to do it.

@alex-the-man
Copy link
Contributor

@jonalter Do you use ideaj?

@jonalter
Copy link
Contributor Author

@alex-the-man yes I do, but I am pretty new to using it.

@alex-the-man
Copy link
Contributor

alex-the-man commented Mar 15, 2017

You can start a specific scalatest test case using the test runner integrated in IntelliJ. You will hit some errors if your environment variables are not set properly. You can set them referencing to pom.xml.

However, for your particular case, you can just add a infinite loop around this block. Let it run overnight to verify the race condition is gone.

@codecov-io
Copy link

codecov-io commented Mar 17, 2017

Codecov Report

Merging #307 into master will increase coverage by 0.01%.
The diff coverage is 81.48%.

@@             Coverage Diff              @@
##             master     #307      +/-   ##
============================================
+ Coverage     70.35%   70.37%   +0.01%     
+ Complexity      689      688       -1     
============================================
  Files            94       94              
  Lines          4912     4925      +13     
  Branches        740      743       +3     
============================================
+ Hits           3456     3466      +10     
- Misses          954      955       +1     
- Partials        502      504       +2
Impacted Files Coverage Δ Complexity Δ
...ala/com/cloudera/livy/repl/SparkRInterpreter.scala 66.01% <81.48%> (+1.72%) 3 <0> (+1) ⬆️
...n/java/com/cloudera/livy/rsc/driver/RSCDriver.java 76.29% <0%> (-1.3%) 39% <0%> (-2%)
...cloudera/livy/repl/StatementProgressListener.scala 66% <0%> (+2%) 0% <0%> (ø) ⬇️
...la/com/cloudera/livy/scalaapi/ScalaJobHandle.scala 55.88% <0%> (+2.94%) 0% <0%> (ø) ⬇️

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 5e6f9ed...88d17ea. Read the comment docs.

@jonalter jonalter force-pushed the LIVY-313 branch 2 times, most recently from 7352af6 to 377ba20 Compare March 17, 2017 18:28
@jonalter
Copy link
Contributor Author

@alex-the-man, thanks for the info!

@jonalter
Copy link
Contributor Author

I ran that one test overnight like you suggested with no failure.

@jonalter
Copy link
Contributor Author

PR ready for review

@alex-the-man
Copy link
Contributor

LGTM. @zjffdu

@zjffdu
Copy link
Contributor

zjffdu commented Mar 23, 2017

LGTM, thanks @jonalter will merge it.

@zjffdu zjffdu merged commit 0de0e28 into cloudera:master Mar 23, 2017
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.

4 participants