[Livy 899] The state of interactive session is always idle when using thrift protocol. #377

jianzhenwu · 2022-12-18T04:15:51Z

What changes were proposed in this pull request?

Should broadcast ReplState to the RSCClient in RSCDriver or the session state is always idle when using thrift protocol.
Add unit test testSessionState in ThriftSessionTest to verify that the state change is as expected.

https://issues.apache.org/jira/browse/LIVY-899

How was this patch tested?

This patch is tested by unit tests. The the state change is recorded in replStateChangedHistoryInTest when testing.

codecov-commenter · 2022-12-18T04:40:44Z

Codecov Report

Merging #377 (d062281) into master (314f2de) will decrease coverage by 36.98%.
The diff coverage is 0.00%.

❗ Current head d062281 differs from pull request most recent head 322af8e. Consider uploading reports for the commit 322af8e to get more accurate results

@@              Coverage Diff              @@
##             master     #377       +/-   ##
=============================================
- Coverage     65.58%   28.61%   -36.98%     
+ Complexity      952      379      -573     
=============================================
  Files           103      103               
  Lines          6044     6057       +13     
  Branches        911      912        +1     
=============================================
- Hits           3964     1733     -2231     
- Misses         1536     3977     +2441     
+ Partials        544      347      -197

Impacted Files	Coverage Δ
...rc/main/java/org/apache/livy/rsc/BaseProtocol.java	`0.00% <ø> (-52.13%)`	⬇️
...c/src/main/java/org/apache/livy/rsc/RSCClient.java	`0.00% <0.00%> (-72.79%)`	⬇️
...ava/org/apache/livy/rsc/driver/JobContextImpl.java	`0.00% <0.00%> (-86.67%)`	⬇️
...in/java/org/apache/livy/rsc/driver/JobWrapper.java	`0.00% <0.00%> (-88.58%)`	⬇️
...ain/java/org/apache/livy/rsc/driver/RSCDriver.java	`0.00% <0.00%> (-80.92%)`	⬇️

... and 82 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mgaido91

I am not sure about the change, as this does not involve only the thriftserver module, but all interactive sessions. If the problem involves all interactive sessions, then it is ok, otherwise the fix is probably not the correct one. Can you check if the same issue affects all interactive jobs? If so, I think we can go ahead with this PR, but I would recommend a different reviewer more familiar with the code in rsc, otherwise, the fix needs to be done only on the thriftserver part and we need more investigation to understand why normal interactive sessions do not have the issue while thriftserver has. Thanks.

mgaido91 · 2022-12-18T10:54:15Z

thriftserver/session/src/test/java/org/apache/livy/thriftserver/session/ThriftSessionTest.java

-import java.util.List;
-import java.util.Properties;
-import java.util.concurrent.TimeUnit;
+import java.util.*;


please avoid importing everything, just add what we need

mgaido91 · 2022-12-18T10:54:41Z

thriftserver/session/src/test/java/org/apache/livy/thriftserver/session/ThriftSessionTest.java

@@ -192,6 +192,27 @@ public void testThriftSession() throws Exception {
    waitFor(new UnregisterSessionJob(s3));
  }

+  @Test
+  public void testSessionState() throws Exception {
+


please remove unneeded blank line

mgaido91 · 2022-12-18T10:56:03Z

thriftserver/session/src/test/java/org/apache/livy/thriftserver/session/ThriftSessionTest.java

@@ -214,7 +235,7 @@ private SqlJob newSqlJob(String session, String stId, String statement) {
   */
  private Exception expectError(Job<?> job, String expected) throws TimeoutException {
    try {
-      livy.submit(job).get(10, TimeUnit.SECONDS);
+      livy.submit(job).get(10, SECONDS);


these are unneeded changes, please avoid them and use TimeUnit.SECONDS in the added code as well

mgaido91 · 2022-12-18T11:25:07Z

thriftserver/session/src/test/java/org/apache/livy/thriftserver/session/ThriftSessionTest.java

@@ -192,6 +192,27 @@ public void testThriftSession() throws Exception {
    waitFor(new UnregisterSessionJob(s3));
  }

+  @Test
+  public void testSessionState() throws Exception {


why are we testing the RSCDriver in thriftserver? All changes are outside the thriftserver, so the UTs should be in rsc as well. This will also avoid adding dependencies to the thriftserver module.

Because I need to verify that the state is correct at the end.

at the end of what? Here we are not going through the thriftserver, so this test should be moved to rsc

leesf · 2022-12-18T11:30:37Z

rsc/src/main/java/org/apache/livy/rsc/driver/RSCDriver.java

+    broadcast(new ReplState(SessionState.Busy$.MODULE$.state()));
+  }
+
+  public void idleIfActiveJobsEmpty(){


public void idleIfActiveJobsEmpty(){ -> public void idleIfActiveJobsEmpty() {

leesf · 2022-12-18T11:31:08Z

thriftserver/session/src/test/java/org/apache/livy/thriftserver/session/ThriftSessionTest.java

 import java.util.concurrent.TimeoutException;
+import static java.util.concurrent.TimeUnit.*;


jianzhenwu · 2022-12-19T02:21:45Z

I am not sure about the change, as this does not involve only the thriftserver module, but all interactive sessions. If the problem involves all interactive sessions, then it is ok, otherwise the fix is probably not the correct one. Can you check if the same issue affects all interactive jobs? If so, I think we can go ahead with this PR, but I would recommend a different reviewer more familiar with the code in rsc, otherwise, the fix needs to be done only on the thriftserver part and we need more investigation to understand why normal interactive sessions do not have the issue while thriftserver has. Thanks.

The same issue doesn't affect interactive jobs which created by REST API. The is because the handle functions are different in the driver. The REST API uses handle(ctx: ChannelHandlerContext, msg: BaseProtocol.ReplJobRequest) in the ReplDriver while thrift API uses handle(ChannelHandlerContext ctx, JobRequest<?> msg) in the RSCDRiver.
ReplDriver changes state when execute code by invoking stateChangedCallback which broadcast the state to all RPC clients.
RSCDriver does not broadcast state to RPC clients.
I think this patch would not affect other interactive jobs except using thrift API.

mgaido91 · 2022-12-19T21:21:47Z

This fix is causing many changes. The submit method is used for many purposes, such as adding jar, files, so I do not think is a good idea to have a fix that has such a large impact on other cases that should not be affected. The current fix would also set to busy when fetching the results, and executing schema operations, and even while unregistering a session...

I think a better place for the fix would be SqlJob, although it is not a perfect solution either. So I would be very happy if someone comes with a better alternative.

… thrift protocol.

jianzhenwu · 2023-06-18T07:19:54Z

This fix is causing many changes. The submit method is used for many purposes, such as adding jar, files, so I do not think is a good idea to have a fix that has such a large impact on other cases that should not be affected. The current fix would also set to busy when fetching the results, and executing schema operations, and even while unregistering a session...

I think a better place for the fix would be SqlJob, although it is not a perfect solution either. So I would be very happy if someone comes with a better alternative.

sorry for the late reply. I have migrated the changes to SqlJob, can you @mgaido91 review the code again?

jianzhenwu requested a review from mgaido91 December 18, 2022 04:44

mgaido91 reviewed Dec 18, 2022

View reviewed changes

leesf reviewed Dec 18, 2022

View reviewed changes

jianzhenwu added 4 commits June 18, 2023 12:01

[LIVY-899] The state of interactive session is always idle when using…

c1ff4a6

… thrift protocol.

add core in thrift-session, or it would not find SessionState

7985799

code format

5cbbdf9

broadcast replState in SqlJob

322af8e

jianzhenwu force-pushed the LIVY-899 branch from d062281 to 322af8e Compare June 18, 2023 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Livy 899] The state of interactive session is always idle when using thrift protocol. #377

[Livy 899] The state of interactive session is always idle when using thrift protocol. #377

jianzhenwu commented Dec 18, 2022

codecov-commenter commented Dec 18, 2022 •

edited

Loading

mgaido91 left a comment

mgaido91 Dec 18, 2022

leesf Dec 18, 2022

mgaido91 Dec 18, 2022

mgaido91 Dec 18, 2022

mgaido91 Dec 18, 2022

jianzhenwu Dec 19, 2022

mgaido91 Dec 19, 2022

leesf Dec 18, 2022

leesf Dec 18, 2022

jianzhenwu commented Dec 19, 2022

mgaido91 commented Dec 19, 2022

jianzhenwu commented Jun 18, 2023

		import java.util.concurrent.TimeoutException;
		import static java.util.concurrent.TimeUnit.*;

[Livy 899] The state of interactive session is always idle when using thrift protocol. #377

Are you sure you want to change the base?

[Livy 899] The state of interactive session is always idle when using thrift protocol. #377

Conversation

jianzhenwu commented Dec 18, 2022

What changes were proposed in this pull request?

How was this patch tested?

codecov-commenter commented Dec 18, 2022 • edited Loading

Codecov Report

mgaido91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianzhenwu commented Dec 19, 2022

mgaido91 commented Dec 19, 2022

jianzhenwu commented Jun 18, 2023

codecov-commenter commented Dec 18, 2022 •

edited

Loading