[LiVY-590] Add dependency to jersey-core#170
Conversation
|
thanks for submitting the PR @akitanaka. I think we should really first understand why you are seeing this issue. I remember I did several tests also on real clusters after that patch and never saw that issue. Moreover, the reason why it was excluded was an incompatibility with the classes needed by the thriftserver for the http mode. IIRC, the thriftserver needed a newer version of the libraries which are in the dependencies of the hadoop module. I think the solution might be to keep the exclusion form the hadoop dependencies and add a dependency on the needed jars in Livy, so that we do not rely on what is part of the hadoop distribution but we have control on it. |
Codecov Report
@@ Coverage Diff @@
## master #170 +/- ##
============================================
- Coverage 68.67% 68.58% -0.09%
+ Complexity 907 904 -3
============================================
Files 100 100
Lines 5666 5666
Branches 850 850
============================================
- Hits 3891 3886 -5
- Misses 1223 1226 +3
- Partials 552 554 +2
Continue to review full report at Codecov.
|
|
Thank you very much for your answer, mgaido91@ In our env, we don't build thrift server, we just build and run livy-server. I think this is the reason that we're seeing the issue in our env. On the other hand, I still think livy-server should have jersey-core jar file.
I agree with the approach. I updated my PR with adding a dependency on the jersey-core jar. |
akitanaka
changed the title
| <artifactId>guava</artifactId> | ||
| </dependency> | ||
|
|
||
| <dependency> |
There was a problem hiding this comment.
why do we need it here too?
There was a problem hiding this comment.
We need to define it here. (If we remove this property, a Livy server package does not contain jersey-core jar.) Using dependencyManagement section of a parent pom.xml, we define this Livy package will depend on jersey-core. And in server/pom.xml, we defined that Livy server actually depends on jersey-core.
| <httpcore.version>4.4.4</httpcore.version> | ||
| <jackson.version>2.9.8</jackson.version> | ||
| <javax.servlet-api.version>3.1.0</javax.servlet-api.version> | ||
| <jersey.version>1.9</jersey.version> |
There was a problem hiding this comment.
well, this is the root cause of the issue and the reason why we had issues and removed the dependency. In the thriftserver part, it is based on the version 1.19. Could you try if using version 1.19 works for you too? In that case we should use that.
There was a problem hiding this comment.
I see. I updated the pr and tested it in my environment. Our Livy tests succeeded with the jersey-core 1.19.
|
Some checks were failed. I think the check failed because we updated jersey-core from 1.9 to 1.19. When I build Livy without adding this PR, the jersey-core version in thriftserver was 1.9 and not 1.19. So, you did not see the failure when you pushed LIVY-502 because the jersey-core version was 1.9 in the CI test. |
|
mmmh, well the |
|
Hello. As far as I tested locally, the the ITs fails when livy-server has Since a thrift/client jar/path is never used in the server side, I think we can have a different jersey-core version for livy-server and thrift/client. Also, livy-server should have a same version of jersey-core package that hadoop-common has. So, I think we should specify jersey-core version in thrift/client and not livy-server. I updated my pull request, now livy-server has
# In my environment, jersey-core version is 1.9 and not 1.19.
|
|
@akitanaka the problem is not with the thtiftserver client. The problem is when you are enabling the thriftserver module, so the thrift server is running in the Livy server and on server side I remember I had issues because in http mode the Hive 3.0 protocol which is the base for the livy thriftserver needed a newer version than 1.9. To give you more reference, you can see here my commit for avoiding issues with http mode for the thriftserver (545a5c3). I am not sure why we are not seeing issues in the CI. As you can see in the commit description, I had to do that in order to avoid problems with http mode for the server side of the thriftserver. But since I don't see UT failures, I can't prove that. I'll try and run this patch on a local env, meanwhile let me cc @vanzin so he can check and maybe run more tests with this patch in order to ensure this doesn't introduces problems. |
|
I have not been able to reproduce any issue with this new PR, but I remember I did have problems with it and it was environment dependent because it depended on the class loading order. Honestly I don't think the current approach is fine. Just reverting that change isn't the right fix IMHO. I saw those files are in |
|
@mgaido91 I haven't been able to reproduce the issue you experienced in LIVY-502, so I'm still not sure what the issue is. (As I added my test result, as far as I checked out a latest Livy code and built the Livy and Livy thrift server module, a What I want to say is I feel the approach in LIVY-502 (#117) was not correct. Since hadoop-client consumes jersey-core (and livy-server consumes hadoop-client) so we should not exclude the dependency from livy-server. If you can give me a test to reproduce the issue you saw when working on #117, I'll test it in my environment. Also, I'm not sure about the glassfish dependency you mentioned in the previous comment... (At least in my PR, I have not mentioned anything about the glassfish dependency) Could you please explain what this is and what do you want me to test ? |
If you check 545a5c3, you'll see that I had to introduce a glassfish dependency, which was incompatible with version 1.9 of jersey. The reason I had to introduce that dependency was to make the thriftserver work also in http mode. To test the thriftserver in http mode, you can build livy with In this configuration, without excluding |
|
This problem persists in the current master. And then I see the exact same issue that @akitanaka describes, i.e. the Livy session remains in Even if, However, If we look inside the In order to keep If I manually add this jar to the class path of the Livy server, then it works as expected. @akitanaka, can you please add |
|
@akitanaka thank you for my side i copied the 2 jar files into livy, then it works fine. cp /opt/hadoop-3.3.0/share/hadoop/common/lib/jersey-core-1.19.jar /opt/livy2/jars/ Thanks |
|
This pull request has been automatically marked as stale because it has had no activity for at least 3 months. If you are still working on this change or plan to move it forward, please leave a comment or push a new commit so we know to keep it open. Otherwise, this PR will be closed automatically in about one month. Thank you for your contribution to Apache Livy! |
5 participants
What changes were proposed in this pull request?
After I upgraded Livy to 0.6.0-incubating, I get following error message when starting livy-server. Also, the livy-server process cannot get the job's appId and status. (See the JIRA for the details)
Comparing livy 0.5.0 and 0.6.0 packages, I noticed that livy-0.6.0 does not have jersey-core jar file. Due to the lack of the jar file, we're getting the error above.
This change was made by a part of the changes in LIVY-502. This pull request reverts the change.
Looks like this issue is not happening in the CI. Unfortunately, I could not get why the issue does not happen in the CI env. (I'd like to double-check that the CI env does not have jersey-core jar.) However, I think we should not exclude jersey-core dependency because livy-server depends on hadoop and hadoop depends on jersey-core (MessageBodyReader).
How was this patch tested?
Tested manually and confirmed that this change can fix the issue we are seeing.