[LIVY-891] Expose the sessionID via the client APIs [WIP] #348
Conversation
| import java.util.concurrent.Future; | ||
| import java.util.concurrent.ScheduledExecutorService; | ||
| import java.util.concurrent.ThreadFactory; | ||
| import java.util.concurrent.*; |
There was a problem hiding this comment.
Please do not use wildcard imports to pull in everything in a package.
There was a problem hiding this comment.
Thank you, @lmccay. I've updated the files. Can you please review the changes.
Codecov Report
@@ Coverage Diff @@
## master #348 +/- ##
============================================
- Coverage 68.50% 68.29% -0.21%
- Complexity 841 843 +2
============================================
Files 103 103
Lines 5940 5968 +28
Branches 898 907 +9
============================================
+ Hits 4069 4076 +7
- Misses 1312 1331 +19
- Partials 559 561 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Left a comment that questions the appropriateness of this as an extension to LivyClient given that RSCClient is throwing an UnsupportedOperationException. Given that it represents half of the implementations of LivyClient, it seems like consumers of the interface shouldn't encounter a runtime exception from a couple new getters. It also strikes me as odd to need to add a new dependency on livy-api to rsc/pom.xml in order to throw and UnsupportedOperationException. There is code across the project using expecting to us the LivyClient interface - maybe I am missing something there.
| replLastActivity = System.nanoTime(); | ||
| } | ||
| replState = msg.state; | ||
| } | ||
| } | ||
| @Override | ||
| public String getSessionAppId(){throw new UnsupportedOperationException();} |
There was a problem hiding this comment.
It seems to me that this is pointing to the fact that it is an inappropriate extension to the interface.
If there are only 2 implementations of the interface and 1 of them throws exceptions for this methods then I don't think they are appropriate for the interface. What is the story here? Session ID isn't known within RSCClient usage?
https://issues.apache.org/jira/browse/LIVY-891
What changes were proposed in this pull request?
Currently getSessionID() is exposed in HttpClient only for testing purposes. Since the internally created session ID is useful to doing operations such as re-connecting to the session, killing the session etc., it will be useful to expose it as a public api in HttpClient and LivyClient. (Include a link to the associated JIRA and make sure to add a link to this pr on the JIRA as well)
How was this patch tested?
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)
Please review https://livy.incubator.apache.org/community/ before opening a pull request.