[SPARK-32057][SQL][test-hive1.2][test-hadoop2.7] ExecuteStatement: cancel and close should not transiently ERROR by alismess-db · Pull Request #28912 · apache/spark

alismess-db · 2020-06-23T19:08:28Z

What changes were proposed in this pull request?

#28671 introduced a change where the order in which CANCELED state for SparkExecuteStatementOperation is set was changed. Before setting the state to CANCELED, cleanup() was called which kills the jobs, causing an exception to be thrown inside execute(). This causes the state to transiently become ERROR before being set to CANCELED. This PR fixes the order.

Why are the changes needed?

Bug: wrong operation state is set.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit test in SparkExecuteStatementOperationSuite.scala.

alismess-db · 2020-06-23T19:08:47Z

cc @juliuszsompolski

SparkQA · 2020-06-23T19:43:49Z

Test build #124431 has finished for PR 28912 at commit be5e2fa.

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

maropu · 2020-06-23T23:21:12Z

also cc: @yaooqinn

HyukjinKwon · 2020-06-24T02:51:47Z

and also @wangyum

SparkQA · 2020-06-24T14:46:08Z

Test build #124488 has finished for PR 28912 at commit 49f4335.

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

juliuszsompolski

Good catch! Thanks, and sorry for the breakage...
Since #28671 went to 3.0, I think this should get backported as fell.

yaooqinn

LGTM

maropu · 2020-06-24T23:23:39Z

retest this please

maropu · 2020-06-24T23:29:00Z

+ (OperationState.CANCELED, (_: SparkExecuteStatementOperation).cancel()),
+ (OperationState.CLOSED, (_: SparkExecuteStatementOperation).close())
+ ).foreach { case (finalState, transition) =>
+ test(s"SPARK-32057 SparkExecuteStatementOperation should not transiently become ERROR " +


nit: drop s in the head.

SparkQA · 2020-06-24T23:57:57Z

Test build #124503 has finished for PR 28912 at commit 49f4335.

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

HyukjinKwon · 2020-06-25T02:38:40Z

retest this please

SparkQA · 2020-06-25T03:11:37Z

Test build #124509 has finished for PR 28912 at commit 49f4335.

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

gatorsmile · 2020-06-25T05:24:29Z

retest this please

HyukjinKwon · 2020-06-25T13:13:37Z

retest this please

SparkQA · 2020-06-25T13:49:22Z

Test build #124514 has finished for PR 28912 at commit 49f4335.

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

maropu · 2020-06-26T00:10:23Z

@alismess-db Looks the valid test failures.

SparkQA · 2020-06-26T10:57:33Z

Test build #124546 has finished for PR 28912 at commit 03b6c93.

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

SparkQA · 2020-06-26T14:24:09Z

Test build #124548 has finished for PR 28912 at commit d68e77b.

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

juliuszsompolski · 2020-06-26T18:00:00Z

sbt.ForkMain$ForkError: org.mockito.exceptions.base.MockitoException: ClassCastException occurred while creating the mockito mock :
class to mock : 'org.apache.hive.service.cli.session.HiveSession', loaded by classloader : 'sun.misc.Launcher$AppClassLoader@77f03bb1'
created class : 'org.mockito.codegen.HiveSession$MockitoMock$1654160678', loaded by classloader : 'net.bytebuddy.dynamic.loading.MultipleParentClassLoader@55c6518c'
proxy instance class : 'org.mockito.codegen.HiveSession$MockitoMock$1654160678', loaded by classloader : 'net.bytebuddy.dynamic.loading.MultipleParentClassLoader@55c6518c'
instance creation by : ObjenesisInstantiator
You might experience classloading issues, please ask the mockito mailing-list.

@gatorsmile @maropu @HyukjinKwon @yaooqinn I have seen a lot of such issues with mockito in Spark tests lately... Do you know if there is some underlying build/test infra issue that causes all of these?

SparkQA · 2020-07-01T11:17:29Z

Test build #124767 has finished for PR 28912 at commit 839f44c.

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

yaooqinn · 2020-07-01T12:35:41Z

The test failures look like that we need to set up a dedicated JVM for this newly added test

maropu · 2020-07-01T14:35:18Z

@alismess-db Could you try to add the test suite in testsWhichShouldRunInTheirOwnDedicatedJvm?
https://github.com/apache/spark/blob/master/project/SparkBuild.scala#L464

SparkQA · 2020-07-01T16:21:37Z

Test build #124798 has finished for PR 28912 at commit 4aaa34b.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

alismess-db · 2020-07-01T17:02:41Z

retest this please

maropu · 2020-07-02T01:11:33Z

retest this please

SparkQA · 2020-07-02T03:38:18Z

Test build #124834 has finished for PR 28912 at commit 4aaa34b.

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

alismess-db · 2020-07-02T09:32:57Z

retest this please

yaooqinn · 2020-07-03T02:23:15Z

retest this please

SparkQA · 2020-07-06T01:03:29Z

Test build #124944 has finished for PR 28912 at commit 4aaa34b.

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

yaooqinn · 2020-07-06T03:32:47Z

The tag test-hive1.2 works wrongly with hadoop-3.2, I opened another PR #29006 to fix that. May you add test-hadoop2.7 to the pr title and retry jenkins? @maropu @alismess-db

HyukjinKwon · 2020-07-06T03:49:22Z

retest this please

maropu · 2020-07-06T06:56:16Z

Ah, I see. we need the tag [test-hadoop2.7], too...

SparkQA · 2020-07-06T07:05:02Z

Test build #125019 has finished for PR 28912 at commit 4aaa34b.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

juliuszsompolski · 2020-07-06T08:33:46Z

retest this please

SparkQA · 2020-07-06T11:24:06Z

Test build #125056 has finished for PR 28912 at commit 4aaa34b.

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

wangyum

retest this please.

HyukjinKwon · 2020-07-07T10:45:35Z

retest this please

SparkQA · 2020-07-07T14:23:28Z

Test build #125210 has finished for PR 28912 at commit 4aaa34b.

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

HyukjinKwon · 2020-07-08T00:28:32Z

Merged to master and branch-3.0.

…ncel and close should not transiently ERROR ### What changes were proposed in this pull request? #28671 introduced a change where the order in which CANCELED state for SparkExecuteStatementOperation is set was changed. Before setting the state to CANCELED, `cleanup()` was called which kills the jobs, causing an exception to be thrown inside `execute()`. This causes the state to transiently become ERROR before being set to CANCELED. This PR fixes the order. ### Why are the changes needed? Bug: wrong operation state is set. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unit test in SparkExecuteStatementOperationSuite.scala. Closes#28912 from alismess-db/execute-statement-operation-cleanup-order. Authored-by: Ali Smesseim <ali.smesseim@databricks.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org> (cherry picked from commit 8b0a54e) Signed-off-by: HyukjinKwon <gurwls223@apache.org>

alismess-db added 2 commits June 23, 2020 20:57

[SPARK-32057] ExecuteStatement: cancel should not transiently ERROR
928cb20

Fix imports and add timeout to eventually
be5e2fa

probot-autolabelerBot added the SQL label Jun 23, 2020

maropu changed the title ~~[SPARK-32057] ExecuteStatement: cancel should not transiently ERROR~~[SPARK-32057][SQL] ExecuteStatement: cancel should not transiently ERRORJun 23, 2020

Clean test and fix close() operation
49f4335

juliuszsompolski approved these changes Jun 24, 2020
View reviewed changes

alismess-db changed the title ~~[SPARK-32057][SQL] ExecuteStatement: cancel should not transiently ERROR~~[SPARK-32057][SQL] ExecuteStatement: cancel and close should not transiently ERRORJun 24, 2020

yaooqinn approved these changes Jun 24, 2020
View reviewed changes

maropu reviewed Jun 24, 2020
View reviewed changes

maropu approved these changes Jun 24, 2020
View reviewed changes

Use regular mock method, see if that helps against classloading issues
03b6c93

Fix test with deep stubs
d68e77b

juliuszsompolski reviewed Jun 26, 2020
View reviewed changes

Comment thread.../test/scala/org/apache/spark/sql/hive/thriftserver/SparkExecuteStatementOperationSuite.scala Outdated

juliuszsompolski approved these changes Jun 26, 2020
View reviewed changes

Comment thread.../test/scala/org/apache/spark/sql/hive/thriftserver/SparkExecuteStatementOperationSuite.scala Outdated

Add empty line
839f44c

Add comment and run in dedicated JVM
4aaa34b

probot-autolabelerBot added the BUILD label Jul 1, 2020

juliuszsompolski mentioned this pull request Jul 3, 2020
[SPARK-26533][SQL][test-hadoop2.7] Support query auto timeout cancel on thriftserver #28991
Closed

yaooqinn mentioned this pull request Jul 6, 2020
[SPARK-32058][BUILD][SQL][test-hive1.2][test-hadoop2.7][FOLLOWUP] Set hadoop 2.7.4 for hive 1.2 profile #29006
Closed

HyukjinKwon changed the title ~~[SPARK-32057][SQL][test-hive1.2] ExecuteStatement: cancel and close should not transiently ERROR~~[SPARK-32057][SQL][test-hive1.2][test-hadoop2.7] ExecuteStatement: cancel and close should not transiently ERRORJul 6, 2020

wangyum reviewed Jul 6, 2020
View reviewed changes

HyukjinKwon approved these changes Jul 8, 2020
View reviewed changes

HyukjinKwon closed this in 8b0a54eJul 8, 2020

Uh oh!

Conversation

alismess-db commented Jun 23, 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!

alismess-db commented Jun 23, 2020

Uh oh!

SparkQA commented Jun 23, 2020

Uh oh!

maropu commented Jun 23, 2020

Uh oh!

HyukjinKwon commented Jun 24, 2020

Uh oh!

SparkQA commented Jun 24, 2020

Uh oh!

juliuszsompolski left a comment

Choose a reason for hiding this comment

Uh oh!

yaooqinn left a comment

Choose a reason for hiding this comment

Uh oh!

maropu commented Jun 24, 2020

Uh oh!

maropuJun 24, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 24, 2020

Uh oh!

HyukjinKwon commented Jun 25, 2020

Uh oh!

SparkQA commented Jun 25, 2020

Uh oh!

gatorsmile commented Jun 25, 2020

Uh oh!

HyukjinKwon commented Jun 25, 2020

Uh oh!

SparkQA commented Jun 25, 2020

Uh oh!

maropu commented Jun 26, 2020

Uh oh!

SparkQA commented Jun 26, 2020

Uh oh!

Uh oh!

SparkQA commented Jun 26, 2020

Uh oh!

Uh oh!

juliuszsompolski commented Jun 26, 2020

Uh oh!

SparkQA commented Jul 1, 2020

Uh oh!

yaooqinn commented Jul 1, 2020

Uh oh!

maropu commented Jul 1, 2020

Uh oh!

SparkQA commented Jul 1, 2020

Uh oh!

alismess-db commented Jul 1, 2020

Uh oh!

maropu commented Jul 2, 2020

Uh oh!

SparkQA commented Jul 2, 2020

Uh oh!

alismess-db commented Jul 2, 2020

Uh oh!

yaooqinn commented Jul 3, 2020

Uh oh!

SparkQA commented Jul 6, 2020

Uh oh!

yaooqinn commented Jul 6, 2020

Uh oh!

HyukjinKwon commented Jul 6, 2020

Uh oh!

maropu commented Jul 6, 2020

Uh oh!

SparkQA commented Jul 6, 2020

Uh oh!

juliuszsompolski commented Jul 6, 2020

wangyum left a comment •
edited
Loading