LIVY-1030 cleanup of context and clients#506
Conversation
edwardcapriolo
force-pushed
the
LIVY-1030
branch
2 times, most recently
from
f361c55 to
e2b933d
Compare
lmccay
left a comment
lmccay
left a comment
There was a problem hiding this comment.
Tests are failing for this pull request.
|
@lmccay Hadoop 3.5.0 is now min java 17. Spark 3.4 has minimum java 17. Lets remove the java8 branch builder and move this project forward. It isn't possible to fix things into the "deep past" |
This requires a larger discussion than here on a specific pull request. It is a discussion that would need to be had on the plans for the next release. I am supportive of it, in general, but again, it needs discussion on the dev@ list. Feel free to start the DISCUSS. |
|
@lmccay Also the errors: at org.apache.livy.thriftserver.LivyThriftSessionManager.$anonfun$openSession$2(LivyThriftSessionManager.scala:269) I think this entire suite is flakey, Just my opinion but they fail sometimes when i run master. They fail randomly quite often. |
We should revisit those then. If there is no JIRA then go ahead and file one and target 0.10.0 release. NOTE: if we drop java 8 altogether at that time, it may be a 1.0.0. There are plenty of PRs that aren't failing these, so trying a rerun should get a pass at some point. I'm going to be out of pocket for a bit - maybe will make it back later this evening but heading out now. |
|
Oh, this is indeed the one that I retried a couple times. |
|
As it relates to Java 8. I used to be a system admin, and I am all about backwards compatibility. But for realism sake: Spark is at 3.5 (17) and 4.X. I find it really hard to believe someone would pick up livy trunk today, and say "I really want to run new shiny living on spark 2.3 and hadoop 2.4 on java 8". Likewise I cant imagine anyone using livy-0.7 on spark2 and then saying "i need this critical bug fix in trunk" I would say branch off an old one. |
Again, this is a community decision and not one to make at a pull request level. Bring it to the dev@ list and drive the DISCUSS, change the PR to pass in current project state or wait for the community to bring it up. It's not getting merged while failing test runs on currently supported Java versions and we aren't dropping support for a java version for a clean up pull request failing tests. There are aspects to consider in dropping the version of java than just dropping support that need to be discussed as well. We will need to determine the near term policy on java 17+ syntax and the effect on backports while previous lines are still technically supported and may need at least security patches. These are the sorts of things that make it a larger discussion. |
Of course, I wouldn't ask you to merge something that is breaking. Have you looked at the PR? All i did was change a method that returns a null to a method that returns Option. Did I actually produce bad code or is this simply flaky builds? I observe these test fail on MASTER edward@fedora:~/incubator-livy$ git log | head -10
commit 284467b98b8f086cb3cf9a6813873141bced5dc2
Author: Edward Capriolo <edlinuxguru@gmail.com>
Date: Fri Jan 16 10:24:14 2026 -0500
LIVY-1029 modernize versions (#505)
[INFO] --- surefire:2.22.1:test (default-test) @ livy-rsc ---
[WARNING] Parameter 'systemProperties' is deprecated: Use systemPropertyVariables instead.
[INFO]
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.livy.rsc.TestJobHandle
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.082 s - in org.apache.livy.rsc.TestJobHandle
[INFO] Running org.apache.livy.rsc.TestSparkClient
[ERROR] Tests run: 19, Failures: 0, Errors: 18, Skipped: 0, Time elapsed: 28.672 s <<< FAILURE! - in org.apache.livy.rsc.TestSparkClient
[ERROR] testSyncRpc(org.apache.livy.rsc.TestSparkClient) Time elapsed: 5.65 s <<< ERROR!
java.util.concurrent.ExecutionException: java.io.IOException: RSCClient instance stopped.
at org.apache.livy.rsc.TestSparkClient.runTest(TestSparkClient.java:593)
at org.apache.livy.rsc.TestSparkClient.runTest(TestSparkClient.java:580)
at org.apache.livy.rsc.TestSparkClient.testSyncRpc(TestSparkClient.java:202)
Caused by: java.io.IOException: RSCClient instance stopped.
It isn't a reason to block my commit because the tests don't run at all :). This is the exact reason I reached out to the incubator-ml to come on as a committer, because I think maybe no one is aware that the builds don't work, or they dont seem to care. |
Well, I built master and those tests pass. I build with your patch applied and it fails. Deterministically, with the error that you show above. Built the same way with the docker build. Have you tried building master without your change? What run is the above output from? I do see other pull requests with the builds passing. Note that the build command that I am using is the following which is also being fixed in the docs as the proper build command [1]: docker run --rm -it -v $(pwd):/workspace -v $HOME/.m2:/root/.m2 livy-ci mvn clean package -Pspark3 -Pscala-2.12 I am leaving your community complaints aside, as we try and work through your contirbutions for the value that they can provide. |
|
[WARNING] Parameter 'systemProperties' is deprecated: Use systemPropertyVariables instead. Clean checkout java 17 first RSC method hands then fails. But if you say it works maybe ill try something else. Don't take it as a "complaint about the community". I am am just presenting what I observer. There are 90 pull requests here, because getting engagement is a challenge. If the project was supper healthy someone might answer back the people who send pr. I am trying to look forward. For example, this project has scalatra, which itself I think is a rather dead scala framework and its a versions maybe 5 years old, It a deep hole. You cant just upgrade anymore, the project has forked and etc etc. I do apprecate that you have run the PRs and I totally admit i could be wrong about the state of some of them, as I switch gears here and there trying to get this or that working. |
edwardcapriolo
force-pushed
the
LIVY-1030
branch
from
e2b933d to
a9d2b8d
Compare
|
@lmccay You are right. I found an issue I created. //Note. Your compiler or IDE may identify this method as unused
//tests fail without it
public void handle(ChannelHandlerContext ctx, RemoteDriverAddress msg) {Note to other developers: The test Running org.apache.livy.rsc.TestSparkClient often creates a fork-bomb situation on my computer by rapidly launching multiple sparks on my system creating memory pressure. But they do run once you movedto a computer with ~16GB of ram or keep your browser closed to save RAM. |
|
Additionally I have created https://issues.apache.org/jira/browse/LIVY-1033. I do not believe I ever had R on my system and my command line invocations never failed. |
lmccay
left a comment
lmccay
left a comment
There was a problem hiding this comment.
@edwardcapriolo - thank you for the additional context through the code review comments. That was very helpful. I've resolved those as accepted. I did come across this one question for you though. I did approve the rerun of the tests and they passed which is great progress!
| try { | ||
| task.run(); | ||
| } finally { | ||
| boolean ignored = confFile.delete(); |
There was a problem hiding this comment.
@edwardcapriolo - is this 'ignored' somehow used? Is it not scoped to this finally block only?
There was a problem hiding this comment.
My IDE will mark unused variables as yellow. You have a few options two of which:
- you could add a log statement
- you can name the variable 'ignored'
I strive to make as many files as possible "fully green". Here i took option 2. Explicitly ignored not ignored because I was unaware of a return.
There was a problem hiding this comment.
Please remove that. I spent time trying to figure out why it was necessary. We don't need your IDE to be fully green here. :)
There was a problem hiding this comment.
I think its good practice. Ignore return values is usually a findbugs warning. Like "mkdir() returns 1 when it fails to make the directory."
https://softdevbuilttolast.wordpress.com/2010/03/23/findbugs-help-create-robust-code-with-the-checkreturnvalue-annotation/
The yellow in my IDE is what called my attention to the initial real null pointer issue in the loop that processes the classpath
There was a problem hiding this comment.
Remove it or replace it with logging which will actually do something. It is not an existing practice in that file to add local vars that have no value and that may look like a bug. I can see a log message at least adding value in case it failed to delete.
There was a problem hiding this comment.
Yes. I agree that is the right coarse of action add a logging message.
lmccay
left a comment
lmccay
left a comment
There was a problem hiding this comment.
@edwardcapriolo - did you just add the bump versions commit to this with this last iteration? Why mix them?
edwardcapriolo
force-pushed
the
LIVY-1030
branch
from
a14aae9 to
a9d2b8d
Compare
I cleaned this up. My bad. Switching branches a often. 2x check for me. |
|
@edwardcapriolo - thanks for your contribution. This has been merged and will be included in the next release off of master! |
Thanks for your help as well. |
2 participants
What changes were proposed in this pull request?
An NPE, some unused variables, and a more clear design for the client factories
https://issues.apache.org/jira/browse/LIVY-1030
How was this patch tested?
Unit tests.
Please review https://livy.incubator.apache.org/community/ before opening a pull request.