Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Jan 21, 2020

What changes were proposed in this pull request?

Increased the limit for log events that could be stored in SparkFunSuite.LogAppender from 100 to 1000.

Why are the changes needed?

Sometimes (see traces in SPARK-30599) additional info is logged via log4j, and appended to LogAppender. For example, unusual log entries are:

[36] Removed broadcast_214_piece0 on 192.168.1.66:52354 in memory (size: 5.7 KiB, free: 2003.8 MiB)
[37] Removed broadcast_204_piece0 on 192.168.1.66:52354 in memory (size: 5.7 KiB, free: 2003.9 MiB)
[38] Removed broadcast_200_piece0 on 192.168.1.66:52354 in memory (size: 3.7 KiB, free: 2003.9 MiB)
[39] Removed broadcast_207_piece0 on 192.168.1.66:52354 in memory (size: 24.2 KiB, free: 2003.9 MiB)
[40] Removed broadcast_208_piece0 on 192.168.1.66:52354 in memory (size: 24.2 KiB, free: 2003.9 MiB)

and a test which uses LogAppender can fail with the exception:

java.lang.IllegalStateException: Number of events reached the limit of 100 while logging CSV header matches to schema w/ enforceSchema.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By re-running "SPARK-23786: warning should be printed if CSV header doesn't conform to schema" in a loop.

@MaxGekk MaxGekk requested a review from srowen January 21, 2020 18:57
@srowen
Copy link
Member

srowen commented Jan 21, 2020

Jenkins test this please

@SparkQA
Copy link

SparkQA commented Jan 21, 2020

Test build #117196 has finished for PR 27312 at commit ed4578d.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class LogAppender(msg: String = \"\", maxEvents: Int = 1000) extends AppenderSkeleton

@MaxGekk
Copy link
Member Author

MaxGekk commented Jan 21, 2020

jenkins, retest this, please

@SparkQA
Copy link

SparkQA commented Jan 21, 2020

Test build #117197 has finished for PR 27312 at commit ed4578d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class LogAppender(msg: String = \"\", maxEvents: Int = 1000) extends AppenderSkeleton

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-30599][CORE][SQL][TESTS] Increase the maximum number of log events in LogAppender [SPARK-30599][CORE][TESTS] Increase the maximum number of log events in LogAppender Jan 21, 2020
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Merged to master.
Thank you, @MaxGekk and @srowen .

@dongjoon-hyun
Copy link
Member

BTW, do we need this in branch-2.4, too?

@SparkQA
Copy link

SparkQA commented Jan 21, 2020

Test build #117201 has finished for PR 27312 at commit ed4578d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class LogAppender(msg: String = \"\", maxEvents: Int = 1000) extends AppenderSkeleton

@maropu
Copy link
Member

maropu commented Jan 23, 2020

late LGTM;

This common subclass has been implemented recently in 88fc8db.
In branch-2.4, since the existing unit tests has the their own log appender instances, I think its less worth backporting this.

@dongjoon-hyun
Copy link
Member

Got it. Thank you for confirmation, @maropu !

@MaxGekk MaxGekk deleted the log-appender-filter branch June 5, 2020 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants