[SPARK-22837][SQL]Session timeout checker does not work in SessionManager. #20025

zuotingbing · 2017-12-20T02:28:08Z

What changes were proposed in this pull request?

Currently we do not call the super.init(hiveConf) in SparkSQLSessionManager.init. So we do not load the config HIVE_SERVER2_SESSION_CHECK_INTERVAL HIVE_SERVER2_IDLE_SESSION_TIMEOUT HIVE_SERVER2_IDLE_SESSION_CHECK_OPERATION , which cause the session timeout checker does not work.

How was this patch tested?

manual tests

…ager.

felixcheung

@gatorsmile

felixcheung · 2017-12-22T07:00:34Z

sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/session/SessionManager.java

    }
  }

+  private void initSessionTimeoutCheckerConfig() {


should we refactor createBackgroundOperationPool to call this?

Thanks @felixcheung . In my opinion you can create a new or follow-up PR if it is necessary. This PR is to fix the bug about the Session Timeout Checker does not work currently.

I asked because these lines are duplicated
https://github.com/zuotingbing/spark/blob/34b7680dfd915449087b8f5541c682b02df5dfbc/sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/session/SessionManager.java#L105

felixcheung · 2017-12-22T07:01:01Z

Jenkins, ok to test

SparkQA · 2017-12-22T07:22:56Z

Test build #85299 has finished for PR 20025 at commit 34b7680.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

zuotingbing · 2017-12-27T01:22:21Z

Could you please merge this to Master? Thanks @felixcheung @srowen

felixcheung

@cloud-fan @gatorsmile would you like a look please?

felixcheung · 2017-12-28T08:20:58Z

sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/session/SessionManager.java

    }
  }

+  private void initSessionTimeoutCheckerConfig() {


I asked because these lines are duplicated
https://github.com/zuotingbing/spark/blob/34b7680dfd915449087b8f5541c682b02df5dfbc/sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/session/SessionManager.java#L105

zuotingbing · 2017-12-28T08:34:07Z

Currently we do not call the super.init(hiveConf) in SparkSQLSessionManager.init, it means SessionManager.init which includes createBackgroundOperationPool() will not be called.

i fix this bug refer to if (hiveConf.getBoolVar(ConfVars.HIVE_SERVER2_LOGGING_OPERATION_ENABLED)) { invoke(classOf[SessionManager], this, "initOperationLogRootDir") } or setSuperField(this, "backgroundOperationPool", Executors.newFixedThreadPool(backgroundPoolSize)) which also include duplicate lines in SessionManager.java

liufengdb · 2017-12-29T01:36:04Z

@zuotingbing I think all the code in SparkSQLSessionManager.scala should be gone because they are just some reflection hacks. It is possible to call super.init(hiveConf) instead to get the session timeout checker and all other things start, isn't?

zuotingbing · 2017-12-29T02:48:40Z

@liufengdb I think the class SessionManager.java is merged from Hive originally, and in Spark we redesigned it by adding SparkSQLSessionManager.scala with no affect to SessionManager.java :
val sparkSqlSessionManager = new SparkSQLSessionManager(hiveServer, sqlContext) setSuperField(this, "sessionManager", sparkSqlSessionManager)

zuotingbing · 2017-12-29T03:00:43Z

@rxin Could you please to review this? Thanks.

In my opinion we can create a new or follow-up PR if refactor is necessary. This PR is to fix the bug about the Session Timeout Checker does not work currently.

liufengdb · 2017-12-29T04:49:45Z

My understanding is that the reflection was used originally because we don't control what is done inside the init method of the super class, given that different hive versions can be used. However, after we inlined the hive code, it is safe to call the init method directly now. This is a cleaner way to fix the referred and other potential bugs, IMO.

zuotingbing · 2018-01-08T03:31:28Z

@gatorsmile What is your opinion about this? Thanks!

zuotingbing · 2018-01-16T00:46:23Z

ping @gatorsmile

zuotingbing · 2018-01-16T09:43:01Z

ping @liufengdb

SparkQA · 2018-01-16T10:48:47Z

Test build #86167 has finished for PR 20025 at commit 162a93f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-01-17T03:56:59Z

Test build #86231 has finished for PR 20025 at commit 79fc11a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

zuotingbing · 2018-01-17T07:32:35Z

@cloud-fan @gatorsmile @liufengdb @felixcheung @srowen @vanzin Is anybody could make further contact and discuss this PR? Thanks!

liufengdb · 2018-01-18T18:18:08Z

sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/session/SessionManager.java

-        keepAliveTime, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>(poolQueueSize),
-        new ThreadFactoryWithGarbageCleanup(threadPoolName));
-    backgroundOperationPool.allowCoreThreadTimeOut(true);
-


I think we can keep this file unchanged. Looks like the hive thread pool is more flexible than the spark one.

yes, i will update it . Thanks!

liufengdb · 2018-01-18T18:19:11Z

@gatorsmile @felixcheung I left one comment, otherwise lgtm.

gatorsmile · 2018-01-18T18:30:55Z

ok to test

SparkQA · 2018-01-18T18:53:12Z

Test build #86349 has finished for PR 20025 at commit 79fc11a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

…ead pool is more flexible than the spark one.

SparkQA · 2018-01-19T03:28:05Z

Test build #86370 has finished for PR 20025 at commit 5b3c06f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-01-19T16:47:46Z

sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/session/SessionManager.java

-import java.util.concurrent.Future;
-import java.util.concurrent.LinkedBlockingQueue;
-import java.util.concurrent.ThreadPoolExecutor;
-import java.util.concurrent.TimeUnit;


revert this back.

yes, thanks

liufengdb · 2018-01-19T18:33:39Z

sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/session/SessionManager.java

    }
    createBackgroundOperationPool();
    addService(operationManager);
-    super.init(hiveConf);


hmm, I think we revert keep this line too.

yes, we could keep super.init() here and remove initCompositeService in SparkSQLSessionManager.init(). thanks

SparkQA · 2018-01-23T04:31:36Z

Test build #86508 has finished for PR 20025 at commit 883deb2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

zuotingbing · 2018-01-24T00:47:26Z

@gatorsmile @liufengdb Anything else? Thanks!

liufengdb · 2018-01-24T17:15:58Z

lgtm

felixcheung

LGTM so long as @gatorsmile approves.

gatorsmile · 2018-01-24T18:07:07Z

Thanks! Merged to master/2.3

…nager. ## What changes were proposed in this pull request? Currently we do not call the `super.init(hiveConf)` in `SparkSQLSessionManager.init`. So we do not load the config `HIVE_SERVER2_SESSION_CHECK_INTERVAL HIVE_SERVER2_IDLE_SESSION_TIMEOUT HIVE_SERVER2_IDLE_SESSION_CHECK_OPERATION` , which cause the session timeout checker does not work. ## How was this patch tested? manual tests Author: zuotingbing <[email protected]> Closes #20025 from zuotingbing/SPARK-22837. (cherry picked from commit bbb87b3) Signed-off-by: gatorsmile <[email protected]>

[SPARK-22837][SQL]Session timeout checker does not work in SessionMan…

34b7680

…ager.

zuotingbing mentioned this pull request Dec 21, 2017

[SPARK-22793][SQL][BACKPORT-2.0]Memory leak in Spark Thrift Server #19989

Closed

felixcheung reviewed Dec 22, 2017

View reviewed changes

felixcheung reviewed Dec 28, 2017

View reviewed changes

fix code review

79fc11a

zuotingbing force-pushed the SPARK-22837 branch from 162a93f to 79fc11a Compare January 17, 2018 03:27

liufengdb reviewed Jan 18, 2018

View reviewed changes

Keep createBackgroundOperationPool unchanged. Looks like the hive thr…

5b3c06f

…ead pool is more flexible than the spark one.

gatorsmile reviewed Jan 19, 2018

View reviewed changes

liufengdb reviewed Jan 19, 2018

View reviewed changes

fix code review

883deb2

felixcheung approved these changes Jan 24, 2018

View reviewed changes

asfgit closed this in bbb87b3 Jan 24, 2018

zuotingbing deleted the SPARK-22837 branch January 25, 2018 02:33

[SPARK-22837][SQL]Session timeout checker does not work in SessionManager. #20025

[SPARK-22837][SQL]Session timeout checker does not work in SessionManager. #20025

Uh oh!

Conversation

zuotingbing commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung commented Dec 22, 2017

Uh oh!

SparkQA commented Dec 22, 2017

Uh oh!

zuotingbing commented Dec 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixcheung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuotingbing commented Dec 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liufengdb commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuotingbing commented Dec 29, 2017

Uh oh!

zuotingbing commented Dec 29, 2017

Uh oh!

liufengdb commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuotingbing commented Jan 8, 2018

Uh oh!

zuotingbing commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuotingbing commented Jan 16, 2018

Uh oh!

SparkQA commented Jan 16, 2018

Uh oh!

SparkQA commented Jan 17, 2018

Uh oh!

zuotingbing commented Jan 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liufengdb commented Jan 18, 2018

Uh oh!

gatorsmile commented Jan 18, 2018

Uh oh!

SparkQA commented Jan 18, 2018

Uh oh!

SparkQA commented Jan 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 23, 2018

Uh oh!

zuotingbing commented Jan 24, 2018

Uh oh!

zuotingbing commented Dec 20, 2017 •

edited

Loading

zuotingbing commented Dec 27, 2017 •

edited

Loading

zuotingbing commented Dec 28, 2017 •

edited

Loading

liufengdb commented Dec 29, 2017 •

edited

Loading

liufengdb commented Dec 29, 2017 •

edited

Loading

zuotingbing commented Jan 16, 2018 •

edited

Loading