[SPARK-30482][SQL][CORE][TESTS] Add sub-class of `AppenderSkeleton` reusable in tests #27166

MaxGekk · 2020-01-10T08:55:36Z

What changes were proposed in this pull request?

In the PR, I propose to define a sub-class of AppenderSkeleton in SparkFunSuite and reuse it from other tests. The class stores incoming LoggingEvent in an array which is available to tests for future analysis of logged events.

Why are the changes needed?

This eliminates code duplication in tests.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By existing test suites - CSVSuite, OptimizerLoggingSuite, JoinHintSuite, CodeGenerationSuite and SQLConfSuite.

…der-skeleton

MaxGekk · 2020-01-10T08:55:57Z

@maropu Please, review the PR.

MaxGekk · 2020-01-10T09:04:45Z

It follows up #27092 (comment)

SparkQA · 2020-01-10T11:04:48Z

Test build #116478 has finished for PR 27166 at commit 088c414.

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

MaxGekk · 2020-01-10T11:19:06Z

jenkins, retest this, please

maropu

Looks fine if the tests passed, thanks, @MaxGekk ! cc: @dongjoon-hyun @HyukjinKwon

SparkQA · 2020-01-10T13:26:41Z

Test build #116484 has finished for PR 27166 at commit 088c414.

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

HyukjinKwon · 2020-01-10T13:37:06Z

retest this please

SparkQA · 2020-01-10T15:39:37Z

Test build #116489 has finished for PR 27166 at commit 088c414.

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

MaxGekk · 2020-01-10T15:49:49Z

I don't think the failure of python tests is related to the PR.

…eton

SparkQA · 2020-01-10T21:07:57Z

Test build #116504 has finished for PR 27166 at commit 6401164.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class FilterStatistics(
class EventLogFileCompactor(
case class CompactionResult(code: CompactionResultCode.Value, compactIndex: Option[Long])
case class AlterNamespaceSetOwner(

maropu · 2020-01-10T22:11:00Z

The related thread: #27162

maropu · 2020-01-10T22:11:11Z

retest this please

SparkQA · 2020-01-10T23:53:57Z

Test build #116507 has finished for PR 27166 at commit 6401164.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class FilterStatistics(
class EventLogFileCompactor(
case class CompactionResult(code: CompactionResultCode.Value, compactIndex: Option[Long])
case class AlterNamespaceSetOwner(

…der-skeleton

SparkQA · 2020-01-11T23:10:37Z

Test build #116539 has finished for PR 27166 at commit 5ede347.

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

dongjoon-hyun · 2020-01-12T03:42:48Z

Retest this please.

SparkQA · 2020-01-12T05:40:33Z

Test build #116542 has finished for PR 27166 at commit 5ede347.

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

MaxGekk · 2020-01-12T07:01:57Z

Not sure how it is related to my changes:

======================================================================
ERROR: test_memory_limit (pyspark.tests.test_worker.WorkerMemoryTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jenkins/workspace/SparkPullRequestBuilder/python/pyspark/tests/test_worker.py", line 193, in test_memory_limit
    actual = rdd.map(lambda _: getrlimit()).collect()
  File "/home/jenkins/workspace/SparkPullRequestBuilder/python/pyspark/rdd.py", line 889, in collect
    sock_info = self.ctx._jvm.PythonRDD.collectAndServe(self._jrdd.rdd())
  File "/home/jenkins/workspace/SparkPullRequestBuilder/python/lib/py4j-0.10.8.1-src.zip/py4j/java_gateway.py", line 1286, in __call__
    answer, self.gateway_client, self.target_id, self.name)
  File "/home/jenkins/workspace/SparkPullRequestBuilder/python/lib/py4j-0.10.8.1-src.zip/py4j/protocol.py", line 328, in get_return_value
    format(target_id, ".", name), value)
Py4JJavaError: An error occurred while calling z:org.apache.spark.api.python.PythonRDD.collectAndServe.
: org.apache.spark.SparkException: Job aborted due to stage failure: Task 0 in stage 0.0 failed 1 times, most recent failure: Lost task 0.0 in stage 0.0 (TID 0, amp-jenkins-worker-05.amp, executor driver): org.apache.spark.SparkException: Python worker exited unexpectedly (crashed)

MaxGekk · 2020-01-12T07:05:37Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/OptimizerLoggingSuite.scala

-    override def requiresLayout(): Boolean = false
-  }
-
-  private def withLogLevelAndAppender(level: Level, appender: Appender)(f: => Unit): Unit = {


This is removed because the method is not used in the test suite.

MaxGekk · 2020-01-12T07:22:57Z

The difference between the master and this PR is some tests could put more log event in log appender like https://github.com/apache/spark/pull/27166/files#diff-5a20f7138b31f3c344a4af7ab6f426daL45 but log appender should be removed at the end of the test. The problem could happen if a log appender is not removed and continue gather other log events.

I will add a check for maximum number of logged events in LogAppender.

SparkQA · 2020-01-12T09:52:07Z

Test build #116549 has finished for PR 27166 at commit a054624.

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

This reverts commit e17793c.

HyukjinKwon · 2020-01-12T10:08:48Z

@MaxGekk, I can't take a close look right now but it might be related to the Jenkins machine itself, and it happens consistently in some machines only. This can be checked via Jenkins log. I plan to check it tomorrow but it might be a good hint for you to investigate further.

SparkQA · 2020-01-12T12:31:02Z

Test build #116553 has finished for PR 27166 at commit 83b4b70.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class MockAppender extends AppenderSkeleton
class MockAppender extends AppenderSkeleton
class MockAppender extends AppenderSkeleton
class MockAppender extends AppenderSkeleton
class TestAppender extends AppenderSkeleton

MaxGekk · 2020-01-12T15:08:15Z

jenkins, retest this, please

SparkQA · 2020-01-12T17:44:54Z

Test build #116557 has finished for PR 27166 at commit 83b4b70.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class MockAppender extends AppenderSkeleton
class MockAppender extends AppenderSkeleton
class MockAppender extends AppenderSkeleton
class MockAppender extends AppenderSkeleton
class TestAppender extends AppenderSkeleton

This reverts commit 83b4b70.

SparkQA · 2020-01-12T20:00:25Z

Test build #116565 has finished for PR 27166 at commit 5fc87c4.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class LogAppender(maxEvents: Int = 100) extends AppenderSkeleton

MaxGekk · 2020-01-12T20:50:00Z

I reverted all the changes of this PR, and python tests passed (#27166 (comment)), but when I reverted everything back - the test failed again (#27166 (comment)). At the moment, I cannot figure out how my changes impact on the python test.

HyukjinKwon · 2020-01-13T09:50:26Z

retest this please

HyukjinKwon · 2020-01-13T09:50:43Z

Should be fixed as of 0823aec ...

SparkQA · 2020-01-13T12:26:26Z

Test build #116628 has finished for PR 27166 at commit 5fc87c4.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class LogAppender(maxEvents: Int = 100) extends AppenderSkeleton

MaxGekk · 2020-01-14T06:43:20Z

@HyukjinKwon 's fix helped, @maropu this can be merged.

maropu · 2020-01-14T07:02:21Z

yea, I will, thanks!

maropu · 2020-01-14T07:03:44Z

Merged to master.

MaxGekk added 8 commits January 9, 2020 23:30

Move LogAppender to SparkFunSuite

e17793c

Reuse LogAppender from SparkFunSuite

8280c65

Dedup CSVSuite

b1c884f

Dedup OptimizerLoggingSuite

4dfdf02

Dedup JoinHintSuite

6f99443

Dedup CodeGenerationSuite

e50febb

Merge remote-tracking branch 'remotes/origin/master' into dedup-appen…

69ff335

…der-skeleton

Dedup SQLConfSuite

088c414

maropu approved these changes Jan 10, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into dedup-appender-skel…

6401164

…eton

Merge remote-tracking branch 'remotes/origin/master' into dedup-appen…

5ede347

…der-skeleton

dongjoon-hyun added SQL SPARK CORE labels Jan 12, 2020

MaxGekk commented Jan 12, 2020

View reviewed changes

MaxGekk added 2 commits January 12, 2020 11:05

Limit the number of event stored in LogAppender

fc20d75

> -> >=

a054624

Revert "Move LogAppender to SparkFunSuite"

83b4b70

This reverts commit e17793c.

Revert "Revert "Move LogAppender to SparkFunSuite""

5fc87c4

This reverts commit 83b4b70.

maropu closed this in 88fc8db Jan 14, 2020

MaxGekk mentioned this pull request Jan 20, 2020

[SPARK-30530][SQL][FOLLOW-UP] Remove unnecessary codes and fix comments accordingly in UnivocityParser #27287

Closed

MaxGekk deleted the dedup-appender-skeleton branch June 5, 2020 19:42

[SPARK-30482][SQL][CORE][TESTS] Add sub-class of AppenderSkeleton reusable in tests #27166

[SPARK-30482][SQL][CORE][TESTS] Add sub-class of AppenderSkeleton reusable in tests #27166

Uh oh!

Conversation

MaxGekk commented Jan 10, 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!

MaxGekk commented Jan 10, 2020

Uh oh!

MaxGekk commented Jan 10, 2020

Uh oh!

SparkQA commented Jan 10, 2020

Uh oh!

MaxGekk commented Jan 10, 2020

Uh oh!

maropu left a comment • 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!

SparkQA commented Jan 10, 2020

Uh oh!

HyukjinKwon commented Jan 10, 2020

Uh oh!

SparkQA commented Jan 10, 2020

Uh oh!

MaxGekk commented Jan 10, 2020

Uh oh!

SparkQA commented Jan 10, 2020

Uh oh!

maropu commented Jan 10, 2020

Uh oh!

maropu commented Jan 10, 2020

Uh oh!

SparkQA commented Jan 10, 2020

Uh oh!

SparkQA commented Jan 11, 2020

Uh oh!

dongjoon-hyun commented Jan 12, 2020

Uh oh!

SparkQA commented Jan 12, 2020

Uh oh!

MaxGekk commented Jan 12, 2020

Uh oh!

MaxGekk Jan 12, 2020

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Jan 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jan 12, 2020

Uh oh!

HyukjinKwon commented Jan 12, 2020

Uh oh!

SparkQA commented Jan 12, 2020

Uh oh!

MaxGekk commented Jan 12, 2020

Uh oh!

SparkQA commented Jan 12, 2020

Uh oh!

SparkQA commented Jan 12, 2020

Uh oh!

MaxGekk commented Jan 12, 2020

Uh oh!

HyukjinKwon commented Jan 13, 2020

Uh oh!

HyukjinKwon commented Jan 13, 2020

Uh oh!

SparkQA commented Jan 13, 2020

Uh oh!

MaxGekk commented Jan 14, 2020

Uh oh!

maropu commented Jan 14, 2020

Uh oh!

maropu commented Jan 14, 2020

Uh oh!

Reviewers

Assignees

[SPARK-30482][SQL][CORE][TESTS] Add sub-class of `AppenderSkeleton` reusable in tests #27166

[SPARK-30482][SQL][CORE][TESTS] Add sub-class of `AppenderSkeleton` reusable in tests #27166

maropu left a comment •

edited

Loading

MaxGekk commented Jan 12, 2020 •

edited

Loading