[SPARK-31387] Handle unknown operation/session ID in HiveThriftServer2Listener #28155

alismess-db · 2020-04-08T18:09:39Z

What changes were proposed in this pull request?

The update methods in HiveThriftServer2Listener now check if the parameter operation/session ID actually exist in the sessionList and executionList respectively. This prevents NullPointerExceptions if the operation or session ID is unknown. Instead, a warning is written to the log.

Also, in HiveSessionImpl.close(), we catch any exception thrown by operationManager.closeOperation. If for any reason this throws an exception, other operations are not prevented from being closed.

Why are the changes needed?

The listener's update methods would throw an exception if the operation or session ID is unknown. In Spark 2, where the listener is called directly, this hampers with the caller's control flow. In Spark 3, the exception is caught by the ListenerBus but results in an uninformative NullPointerException.

In HiveSessionImpl.close(), if an exception is thrown when closing an operation, all following operations are not closed.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit tests

juliuszsompolski · 2020-04-08T18:23:53Z

cc @wangyum

In Spark 2, where the listener is called directly, this hampers with the caller's control flow.

This exception could happen if an operation in ERROR or CANCELED state that was never closed by the client was already evicted from the listener state (as not-active) when the session was later closed.

juliuszsompolski · 2020-04-08T18:25:35Z

It is more serious in Spark 2, where the exception from the listener would bubble up all the way to session, but IMO we could also catch it in Spark 3 listener, as this PR proposes.

juliuszsompolski · 2020-04-09T15:01:37Z

...rver/src/test/scala/org/apache/spark/sql/hive/service/cli/session/HiveSessionImplSuite.scala

https://github.com/apache/spark/pull/28155/checks?check_run_id=571648817

[ERROR] [Error] /home/runner/work/spark/spark/sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/service/cli/session/HiveSessionImplSuite.scala:22: object rpc is not a member of package org.apache.hive.service [ERROR] [Error] /home/runner/work/spark/spark/sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/service/cli/session/HiveSessionImplSuite.scala:37: not found: value TProtocolVersion [ERROR] two errors found [ERROR] Failed to execute goal net.alchim31.maven:scala-maven-plugin:4.3.0:testCompile (scala-test-compile-first) on project spark-hive-thriftserver_2.12: Execution scala-test-compile-first of goal net.alchim31.maven:scala-maven-plugin:4.3.0:testCompile failed.: CompileFailed -> [Help 1]

You can use ThriftserverShimUtils.testedProtocolVersions(0) to make it work with both hive-1.2 and hive-2.3 profile.
You must import TProtocolVersion from

…2Listener

juliuszsompolski · 2020-05-11T09:57:17Z

@wangyum @gatorsmile could you take a look?
It improves the robustness of Thriftserver session / Thriftserver listener against uncaught exceptions.
In Spark 2.4, when the listener was running in the same thread as execution, this could cause some sessions to not be properly cleaned up if an exception was thrown from one of the operations in the session during session close.

juliuszsompolski

LGTM

gatorsmile · 2020-05-11T16:23:15Z

ok to test

SparkQA · 2020-05-11T17:04:18Z

Test build #122514 has finished for PR 28155 at commit 3a842b6.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class HiveSessionImplSuite extends SparkFunSuite

gatorsmile · 2020-05-12T16:10:40Z

LGTM

Thanks! Merged to master/3.0

…2Listener ### What changes were proposed in this pull request? The update methods in HiveThriftServer2Listener now check if the parameter operation/session ID actually exist in the `sessionList` and `executionList` respectively. This prevents NullPointerExceptions if the operation or session ID is unknown. Instead, a warning is written to the log. Also, in HiveSessionImpl.close(), we catch any exception thrown by `operationManager.closeOperation`. If for any reason this throws an exception, other operations are not prevented from being closed. ### Why are the changes needed? The listener's update methods would throw an exception if the operation or session ID is unknown. In Spark 2, where the listener is called directly, this hampers with the caller's control flow. In Spark 3, the exception is caught by the ListenerBus but results in an uninformative NullPointerException. In HiveSessionImpl.close(), if an exception is thrown when closing an operation, all following operations are not closed. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Unit tests Closes #28155 from alismess-db/hive-thriftserver-listener-update-safer. Authored-by: Ali Smesseim <[email protected]> Signed-off-by: gatorsmile <[email protected]> (cherry picked from commit 6994c64) Signed-off-by: gatorsmile <[email protected]>

dongjoon-hyun · 2020-05-14T18:12:20Z

Hi, @alismess-db , @juliuszsompolski , @gatorsmile .

Is this tested with Hive 1.2 profile?

According to the Jenkins result, this seems to break Hive 1.2 profile in branch-3.0. Could you take a look at this?

https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-branch-3.0-test-maven-hadoop-2.7-hive-1.2/324/testReport/org.apache.spark.sql.hive.thriftserver/SparkSQLEnvSuite/SPARK_29604_external_listeners_should_be_initialized_with_Spark_classloader/history/

dongjoon-hyun · 2020-05-14T18:51:06Z

Ur, I found that this breaks hive-2.3, too. In short, this seems to break all Maven tests in branch-3.0.

dongjoon-hyun · 2020-05-14T19:00:36Z

Ur, it turns out that master branch Maven jobs are broken, too.

I'll revert this patch from master/3.0. Please make another PR and test with Maven since Maven is our official build system.

dongjoon-hyun · 2020-05-15T01:53:41Z

FYI, two JDK11 jobs are recovered. I'm looking at the other failures like #28532 .

…2Listener ### What changes were proposed in this pull request? The update methods in HiveThriftServer2Listener now check if the parameter operation/session ID actually exist in the `sessionList` and `executionList` respectively. This prevents NullPointerExceptions if the operation or session ID is unknown. Instead, a warning is written to the log. Also, in HiveSessionImpl.close(), we catch any exception thrown by `operationManager.closeOperation`. If for any reason this throws an exception, other operations are not prevented from being closed. ### Why are the changes needed? The listener's update methods would throw an exception if the operation or session ID is unknown. In Spark 2, where the listener is called directly, this hampers with the caller's control flow. In Spark 3, the exception is caught by the ListenerBus but results in an uninformative NullPointerException. In HiveSessionImpl.close(), if an exception is thrown when closing an operation, all following operations are not closed. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Unit tests Closes apache#28155 from alismess-db/hive-thriftserver-listener-update-safer. Authored-by: Ali Smesseim <[email protected]> Signed-off-by: gatorsmile <[email protected]> (cherry picked from commit 6994c64) Signed-off-by: Ali Smesseim <[email protected]>

…erver2Listener ### What changes were proposed in this pull request? This is a recreation of #28155, which was reverted due to causing test failures. The update methods in HiveThriftServer2Listener now check if the parameter operation/session ID actually exist in the `sessionList` and `executionList` respectively. This prevents NullPointerExceptions if the operation or session ID is unknown. Instead, a warning is written to the log. To improve robustness, we also make the following changes in HiveSessionImpl.close(): - Catch any exception thrown by `operationManager.closeOperation`. If for any reason this throws an exception, other operations are not prevented from being closed. - Handle not being able to access the scratch directory. When closing, all `.pipeout` files are removed from the scratch directory, which would have resulted in an NPE if the directory does not exist. ### Why are the changes needed? The listener's update methods would throw an exception if the operation or session ID is unknown. In Spark 2, where the listener is called directly, this changes the caller's control flow. In Spark 3, the exception is caught by the ListenerBus but results in an uninformative NullPointerException. In HiveSessionImpl.close(), if an exception is thrown when closing an operation, all following operations are not closed. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Unit tests Closes #28544 from alismess-db/hive-thriftserver-listener-update-safer-2. Authored-by: Ali Smesseim <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…erver2Listener ### What changes were proposed in this pull request? This is a recreation of #28155, which was reverted due to causing test failures. The update methods in HiveThriftServer2Listener now check if the parameter operation/session ID actually exist in the `sessionList` and `executionList` respectively. This prevents NullPointerExceptions if the operation or session ID is unknown. Instead, a warning is written to the log. To improve robustness, we also make the following changes in HiveSessionImpl.close(): - Catch any exception thrown by `operationManager.closeOperation`. If for any reason this throws an exception, other operations are not prevented from being closed. - Handle not being able to access the scratch directory. When closing, all `.pipeout` files are removed from the scratch directory, which would have resulted in an NPE if the directory does not exist. ### Why are the changes needed? The listener's update methods would throw an exception if the operation or session ID is unknown. In Spark 2, where the listener is called directly, this changes the caller's control flow. In Spark 3, the exception is caught by the ListenerBus but results in an uninformative NullPointerException. In HiveSessionImpl.close(), if an exception is thrown when closing an operation, all following operations are not closed. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Unit tests Closes #28544 from alismess-db/hive-thriftserver-listener-update-safer-2. Authored-by: Ali Smesseim <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit d40ecfa) Signed-off-by: Dongjoon Hyun <[email protected]>

juliuszsompolski reviewed Apr 9, 2020

View reviewed changes

alismess-db added 2 commits May 8, 2020 18:32

[SPARK-31387] Handle unknown operation/session id in HiveThriftServer…

47bfa15

…2Listener

Do not depend on TProtocolVersion directly, and fix test

3a842b6

alismess-db force-pushed the hive-thriftserver-listener-update-safer branch from e65424e to 3a842b6 Compare May 9, 2020 14:25

probot-autolabeler bot added SQL WEB UI labels May 9, 2020

juliuszsompolski approved these changes May 11, 2020

View reviewed changes

gatorsmile closed this in 6994c64 May 12, 2020

alismess-db mentioned this pull request May 15, 2020

[SPARK-31387][SQL] Handle unknown operation/session ID in HiveThriftServer2Listener #28544

Closed

[SPARK-31387] Handle unknown operation/session ID in HiveThriftServer2Listener #28155

[SPARK-31387] Handle unknown operation/session ID in HiveThriftServer2Listener #28155

Uh oh!

Conversation

alismess-db commented Apr 8, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

juliuszsompolski commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juliuszsompolski commented Apr 8, 2020

Uh oh!

juliuszsompolski Apr 9, 2020

Choose a reason for hiding this comment

Uh oh!

juliuszsompolski commented May 11, 2020

Uh oh!

juliuszsompolski left a comment

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented May 11, 2020

Uh oh!

SparkQA commented May 11, 2020

Uh oh!

gatorsmile commented May 12, 2020

Uh oh!

dongjoon-hyun commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented May 14, 2020

Uh oh!

dongjoon-hyun commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented May 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

juliuszsompolski commented Apr 8, 2020 •

edited

Loading

dongjoon-hyun commented May 14, 2020 •

edited

Loading

dongjoon-hyun commented May 14, 2020 •

edited

Loading