[SPARK-25338][Test] Ensure to call super.beforeAll() and super.afterAll() in test cases by kiszk · Pull Request #22337 · apache/spark

kiszk · 2018-09-05T06:05:47Z

What changes were proposed in this pull request?

This PR ensures to call super.afterAll() in override afterAll() method for test suites.

Some suites did not call super.afterAll()
Some suites may call super.afterAll() only under certain condition
Others never call super.afterAll().

This PR also ensures to call super.beforeAll() in override beforeAll() for test suites.

How was this patch tested?

Existing UTs

SparkQA · 2018-09-05T07:05:01Z

Test build #95698 has finished for PR 22337 at commit a429ddb.

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

kiszk · 2018-09-05T07:17:58Z

retest this please

SparkQA · 2018-09-05T08:33:31Z

Test build #95703 has finished for PR 22337 at commit a429ddb.

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

dongjoon-hyun · 2018-09-06T04:06:02Z

Hi, @kiszk . Oh, did you check them all? Are these all of them?

dongjoon-hyun · 2018-09-06T04:14:22Z

It seems that we need try...finally for _sc.stop().

dongjoon-hyun · 2018-09-06T04:24:27Z

Here, too. Let's use try to make it sure the invocation of super.afterAll().

dongjoon-hyun · 2018-09-06T04:25:22Z

dongjoon-hyun · 2018-09-06T04:28:37Z

Could you check the other suites like ExternalAppendOnlyUnsafeRowArraySuite and TakeOrderedAndProjectSuite, too? Then, we can remove [kafka][kinesis][flume] from the title.

kiszk · 2018-09-06T04:59:54Z

Sure, I just focused on files under external. Let me address other files, too.

SparkQA · 2018-09-06T14:55:33Z

Test build #95748 has finished for PR 22337 at commit 32db2fc.

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

kiszk · 2018-09-06T16:29:55Z

cc @dongjoon-hyun

dongjoon-hyun · 2018-09-07T05:58:34Z

Oh, this one is beforeAll. Did you check beforeAll, too?

I see. Let me check beforeAll.

dongjoon-hyun · 2018-09-07T06:00:48Z

Is it correct in this test suite context? We usually put super.afterAll() into finally. If there is no reason, let's fix this by switch them.

In general, I agree with your opinion. I found some exceptional cases like this. Thus, I keep the original order.
Should we keep the original order or try to swap them?

IMHO this is clearly an error and we should swap it

dongjoon-hyun · 2018-09-07T06:02:42Z

Could you check the history of this too? Is there a reason for this StateStoreRDDSuite to put deleteRecursively at the end? For the other state store suite, super.afterAll is consistently before delete...

I cannot find other state store suite. Could you please let me know the name of these suites?
I intentionally changed the order since I saw this order (deleteRecursively() ->super.afterAll()) in the following other places.
Since I am neutral on this order, it is ok to keep the original order like
try { super.afterAll() } finally { Utils.deleteRecursively(new File(tempDir)) }
core/src/test/scala/org/apache/spark/rdd/RDDSuite.scala core/src/test/scala/org/apache/spark/storage/DiskBlockManagerSuite.scala sql/core/src/test/scala/org/apache/spark/sql/sources/CreateTableAsSelectSuite.scala sql/core/src/test/scala/org/apache/spark/sql/sources/InsertSuite.scala mllib/src/test/scala/org/apache/spark/ml/source/libsvm/LibSVMRelationSuite.scala mllib/src/test/scala/org/apache/spark/ml/util/MLTest.scala mllib/src/test/scala/org/apache/spark/ml/util/TempDirectory.scala streaming/src/test/scala/org/apache/spark/streaming/rdd/MapWithStateRDDSuite.scala

I prefer your change, too. But, the reason why I asked is that you didn't fix the order in FlatMapGroupsWithStateSuite in this PR. I want to make it sure whether there is a obvious reason or not.

Sure, let me change the order in FlatMapGroupsWithStateSuite , too.

Ur, are you sure? StreamingAggregationSuite, StreamTest, StreamingAggregationSuite and StreamingDeduplicationSuite also have reverse order for StateStore.stop().

Hmm, I see. Let me keep the original order as
try { super.afterAll() } finally { Utils.deleteRecursively(...) }
Thank you for listing them out.

dilipbiswal · 2018-09-07T17:30:19Z

@kiszk Just wanted to understand. Is there a reason that its not under finally block ?

Good catch, added try-finally

dilipbiswal · 2018-09-07T17:30:47Z

@kiszk same question as above.

dilipbiswal · 2018-09-07T17:32:42Z

@kiszk Can sc.stop throw an exception ? If so, shouldn't we attempt to do a teardown first before calling super.afterAll() ?

I'm confused by your comment, it looks like we try and stop the spark context first. Are you suggesting that we want to tear down kafka test utils even if the Spark stop context fails?

@holdenk Please correct me on this one, in my understanding, we do 3 distinct things in the afterAll() method.
1) sc.stop 2) kafkaTestUtils.teardown() 3) super.afterAll()
Currently, even if we fail in (1) or (2) , we will always do (3) as its done in the finally block. My question was , if we fail at (1) , should we do (2) and (3) ? I don't know about the details of the test suite to know for sure if this matters or not. I was just curious.

sc.stop can throw an exception. Thus, I updated this using nested try-finally.

dilipbiswal · 2018-09-07T17:33:07Z

@kiszk Can sc.stop throw an exception ? If so, shouldn't we attempt to do a teardown first before calling super.afterAll() ?

dilipbiswal · 2018-09-07T17:33:31Z

@kiszk same question as above.

SparkQA · 2018-09-07T22:28:19Z

Test build #95805 has finished for PR 22337 at commit 45c7a10.

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

SparkQA · 2018-09-08T07:05:02Z

Test build #95822 has finished for PR 22337 at commit a314776.

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

SparkQA · 2018-09-08T07:05:02Z

Test build #95821 has finished for PR 22337 at commit 6a4cbcf.

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

kiszk · 2018-09-08T14:47:02Z

retest this please

SparkQA · 2018-09-08T18:50:16Z

Test build #95832 has finished for PR 22337 at commit a314776.

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

SparkQA · 2018-09-08T20:11:58Z

Test build #95833 has finished for PR 22337 at commit 309e265.

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

dongjoon-hyun · 2018-09-09T23:39:57Z

@kiszk . Let's remove with BeforeAndAfterAll and the whole afterAll(). These are redundant because we fixed afterAll in StreamTest already.

dongjoon-hyun · 2018-09-09T23:41:01Z

Here, too. Let's remove the redundant with BeforeAndAfterAll and the whole afterAll function.

dongjoon-hyun · 2018-09-09T23:41:50Z

Here, too. with BeforeAndAfterAll and override def afterAll() are redundant.

SparkQA · 2018-09-11T00:53:25Z

Test build #95890 has finished for PR 22337 at commit 2d9e34a.

This patch fails from timeout after a configured wait of `400m`.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FlatMapGroupsWithStateSuite extends StateStoreMetricsTest
class StreamingAggregationSuite extends StateStoreMetricsTest with Assertions
class StreamingDeduplicationSuite extends StateStoreMetricsTest

kiszk · 2018-09-11T01:46:18Z

retest this please

SparkQA · 2018-09-11T05:57:16Z

Test build #95909 has finished for PR 22337 at commit 2d9e34a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FlatMapGroupsWithStateSuite extends StateStoreMetricsTest
class StreamingAggregationSuite extends StateStoreMetricsTest with Assertions
class StreamingDeduplicationSuite extends StateStoreMetricsTest

dongjoon-hyun · 2018-09-11T06:01:56Z

Retest this please.

SparkQA · 2018-09-11T07:05:02Z

Test build #95917 has finished for PR 22337 at commit 2d9e34a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FlatMapGroupsWithStateSuite extends StateStoreMetricsTest
class StreamingAggregationSuite extends StateStoreMetricsTest with Assertions
class StreamingDeduplicationSuite extends StateStoreMetricsTest

kiszk · 2018-09-11T12:08:56Z

retest this please

SparkQA · 2018-09-11T16:45:23Z

Test build #95938 has finished for PR 22337 at commit 2d9e34a.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FlatMapGroupsWithStateSuite extends StateStoreMetricsTest
class StreamingAggregationSuite extends StateStoreMetricsTest with Assertions
class StreamingDeduplicationSuite extends StateStoreMetricsTest

kiszk · 2018-09-11T16:56:37Z

retest this please

dongjoon-hyun

+1, LGTM.

ghost · 2018-09-11T22:17:20Z

Test build #95958 has finished for PR 22337 at commit 2d9e34a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FlatMapGroupsWithStateSuite extends StateStoreMetricsTest
class StreamingAggregationSuite extends StateStoreMetricsTest with Assertions
class StreamingDeduplicationSuite extends StateStoreMetricsTest

ghost · 2018-09-12T21:07:15Z

Ur, could you resolve the conflicts, @kiszk ?

ghost · 2018-09-13T18:05:24Z

Test build #96039 has finished for PR 22337 at commit 53d1ba2.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class FlatMapGroupsWithStateSuite extends StateStoreMetricsTest
class StreamingAggregationSuite extends StateStoreMetricsTest with Assertions
class StreamingDeduplicationSuite extends StateStoreMetricsTest

ghost · 2018-09-13T18:33:17Z

Thank you, @kiszk .
Merged to master.

dongjoon-hyun reviewed Sep 6, 2018
View reviewed changes

kiszk changed the title ~~[SPARK-25338][Test][kafka][kinesis][flume] Ensure to call super.afterAll() in afterAll method in test cases~~[SPARK-25338][Test] Ensure to call super.afterAll() in afterAll method in test casesSep 6, 2018

dongjoon-hyun reviewed Sep 7, 2018
View reviewed changes

kiszk changed the title ~~[SPARK-25338][Test] Ensure to call super.afterAll() in afterAll method in test cases~~[SPARK-25338][Test] Ensure to call super.beforeAll() and super.afterAll() in test casesSep 7, 2018

srowen approved these changes Sep 7, 2018
View reviewed changes

dilipbiswal reviewed Sep 7, 2018
View reviewed changes

srowen approved these changes Sep 9, 2018
View reviewed changes

dongjoon-hyun reviewed Sep 9, 2018
View reviewed changes

dongjoon-hyun approved these changes Sep 11, 2018
View reviewed changes

Kazuaki Ishizaki added 8 commits September 13, 2018 21:53

initial commit
f62e4cd

address review comments
3452edf

address review comment
dc628e9

address review comments
aca9617

change place of nullification
d3c653c

address review comment
e2dff11

address review comment
aa45757

address review comment
53d1ba2

kiszk force-pushed the SPARK-25338 branch from 2d9e34a to 53d1ba2Compare September 13, 2018 13:00

asfgit closed this in f60cd7cSep 13, 2018

Uh oh!

Conversation

kiszk commented Sep 5, 2018• 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!

SparkQA commented Sep 5, 2018

Uh oh!

kiszk commented Sep 5, 2018

Uh oh!

SparkQA commented Sep 5, 2018

Uh oh!

dongjoon-hyun commented Sep 6, 2018

Uh oh!

dongjoon-hyunSep 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dongjoon-hyun commented Sep 6, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiszk commented Sep 6, 2018

Uh oh!

SparkQA commented Sep 6, 2018

Uh oh!

kiszk commented Sep 6, 2018

Uh oh!

dongjoon-hyunSep 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyunSep 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dongjoon-hyunSep 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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 Sep 7, 2018

Uh oh!

kiszk commented Sep 5, 2018•
edited
Loading

dongjoon-hyunSep 6, 2018•
edited
Loading

dongjoon-hyun commented Sep 6, 2018•
edited
Loading

dongjoon-hyunSep 7, 2018•
edited
Loading

dongjoon-hyunSep 7, 2018•
edited
Loading

dongjoon-hyunSep 7, 2018•
edited
Loading

ghost commented Sep 13, 2018•
edited by dongjoon-hyun
Loading