[SPARK-30481][DOCS][FOLLOWUP] Document event log compaction into new section of monitoring.md #27398

HeartSaVioR · 2020-01-30T08:16:26Z

What changes were proposed in this pull request?

This is a FOLLOW-UP PR for review comment on #27208 : #27208 (review)

This PR documents a new feature Eventlog Compaction into the new section of monitoring.md, as it only has one configuration on the SHS side and it's hard to explain everything on the description on the single configuration.

Why are the changes needed?

Event log compaction lacks the documentation for what it is and how it helps. This PR will explain it.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Built docs via jekyll.

change on the new section

change on the table

…section of monitor.md

SparkQA · 2020-01-30T08:36:51Z

Test build #117556 has finished for PR 27398 at commit 49f51ff.

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

SparkQA · 2020-01-30T08:44:45Z

Test build #117557 has finished for PR 27398 at commit 90a3f82.

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

HeartSaVioR · 2020-01-30T08:45:49Z

cc. @vanzin @squito @gaborgsomogyi

dongjoon-hyun · 2020-01-30T17:08:42Z

Thank you for attaching the screenshot, @HeartSaVioR .

docs/monitoring.md

SparkQA · 2020-01-31T06:23:24Z

Test build #117623 has finished for PR 27398 at commit 3e74e05.

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

gaborgsomogyi · 2020-02-03T08:47:00Z

docs/monitoring.md

+
+When the compaction happens, History Server lists all the available event log files, and considers the event log files older than
+retained log files as a target of compaction. For example, if the application A has 5 event log files and
+<code>spark.history.fs.eventLog.rolling.maxFilesToRetain</code> is set to 2, first 3 log files will be selected to be compacted.


s/set to 2, first 3/set to 2, then the first 3/

gaborgsomogyi · 2020-02-03T08:49:25Z

docs/monitoring.md

+retained log files as a target of compaction. For example, if the application A has 5 event log files and
+<code>spark.history.fs.eventLog.rolling.maxFilesToRetain</code> is set to 2, first 3 log files will be selected to be compacted.
+
+Once it selects the files, it analyzes these files to figure out which events can be excluded, and rewrites these files


No specific recommendation but I have the feeling that files term is a little bit overused here. Maybe some rephrasing would be good.

I'll try to rephrase - maybe we can refer as "target" or "candidates" instead of "files".

gaborgsomogyi · 2020-02-03T08:52:28Z

docs/monitoring.md

+<code>spark.history.fs.eventLog.rolling.maxFilesToRetain</code> is set to 2, first 3 log files will be selected to be compacted.
+
+Once it selects the files, it analyzes these files to figure out which events can be excluded, and rewrites these files
+into one compact file with discarding some events. Once rewriting is done, original log files will be deleted.


original log files will be deleted

Maybe worth to mention its best effort? Not yet gone through the merged functionality so just asking what will happen such garbage in the next compaction round?

Yeah it wouldn't matter for the entire logic as listing event log files would take the "last" compact file, and the right side of event log files. But we don't retry deleting them. I'd agree to worth to mention the deletion is best effort.

gaborgsomogyi · 2020-02-03T08:57:15Z

docs/monitoring.md

+* Events for the executor which is terminated
+* Events for the SQL execution which is finished, and related job/stage/tasks events
+
+but the details can be changed afterwards.


but the details can be changed afterwards

Maybe not needed to mention this. Until history server is able to read the files in a backward compatible way (and can provide correct UI result) it's not really relevant whether it's changed between versions or not.

OK agreed. Once we change the logic we may just need to change here.

gaborgsomogyi · 2020-02-03T08:59:24Z

docs/monitoring.md

+but the details can be changed afterwards.
+
+Please note that Spark History Server may not compact the old event log files if figures out not a lot of space
+would be reduced during compaction. For streaming query (including Structured Streaming) we normally expect compaction


Not yet see why including Structured Streaming is needed? That's the main streaming engine.

OK looks redundant. Thanks!

gaborgsomogyi · 2020-02-03T09:01:04Z

docs/monitoring.md

+will run as each micro-batch will trigger one or more jobs which will be finished shortly, but compaction won't run
+in many cases for batch query.
+
+Please also note that this is a new feature introduced in Spark 3.0, and may not be completely stable. In some circumstance,


s/In some circumstance/Under certain circumstances?

gaborgsomogyi · 2020-02-03T09:01:38Z

docs/monitoring.md

+
+Please also note that this is a new feature introduced in Spark 3.0, and may not be completely stable. In some circumstance,
+the compaction may exclude more events than you expect, leading some UI issues on History Server for the application.
+Use with caution.


s/Use with caution/Use it with caution/

gaborgsomogyi · 2020-02-03T09:04:07Z

docs/monitoring.md

+
+but the details can be changed afterwards.
+
+Please note that Spark History Server may not compact the old event log files if figures out not a lot of space


This paragraph not really answers to me why not always compact.

We already described which events are the candidates in above, so it's saying "Please note that Spark History Server may not compact the old event log files if figures out not a lot of space would be reduced during compaction because these event log files majorly fill with running jobs or SQL executions."

Does it answer your question?

tgravescs · 2020-02-03T17:31:26Z

docs/monitoring.md


+### Applying compaction of old event log files
+
+A long-running streaming application can bring a huge single event log file which may cost a lot to maintain and


I assume this works for any application, not just streaming? If so perhaps we change the wording a little to say for large application, and give streaming as example. I used to see long running graph processing applications create huge log files as well.
I also think we should describe what compaction is here up front because if I just read this, I would assume its some sort of compression, but really its throwing away parts of the files.

I assume this works for any application, not just streaming?

You're right. I focused too much on the target of compaction which is most likely streaming application, but in this sentence it's not only for streaming.

I also think we should describe what compaction is here up front

Uh, actually we don't have explicit section for rolling event log, hence I feel it's good to explain what's rolling event log first, and what is "compaction". Otherwise maybe good to have individual section for rolling event log?

tgravescs · 2020-02-03T17:39:03Z

docs/monitoring.md

+logs, via setting the configuration <code>spark.history.fs.eventLog.rolling.maxFilesToRetain</code> on the
+Spark History Server.
+
+When the compaction happens, History Server lists all the available event log files, and considers the event log files older than


"older than retained log files" .. This wording is confusing me a bit because you say "older". Below the config is maxFilesToRetain, is not date based, its a number of files. Am I missing something here or should the wording be somethign like like considers the event log files when there are more than spark.history.fs.eventLog.rolling.maxFilesToRetain? Or is the intention that when there are more than spark.history.fs.eventLog.rolling.maxFilesToRetain, then it looks at the oldest of those log files?

I chose "older" because the target event log files are created "before" than the event log files which will be retained - so that's semantically date based. It might be better to explain it with "index" which is clearer, but requires another explanation on "index".

tgravescs · 2020-02-03T17:39:27Z

docs/monitoring.md

+logs, via setting the configuration <code>spark.history.fs.eventLog.rolling.maxFilesToRetain</code> on the
+Spark History Server.
+
+When the compaction happens, History Server lists all the available event log files, and considers the event log files older than


nit: "the History Server"

tgravescs · 2020-02-03T17:45:56Z

docs/monitoring.md

+* Events for the executor which is terminated
+* Events for the SQL execution which is finished, and related job/stage/tasks events
+
+but the details can be changed afterwards.


should we add in a sentence saying once the events are removed you will no longer be able to view in UI - or if there is anything consequence of this happening.

I thought the effect is intuitive as we "exclude" events during rewriting, but if explicitly mentioning would make it clearer, let's do it.

HeartSaVioR · 2020-02-07T05:55:28Z

Thanks for the detailed reviews and sorry to not address review comments so far. There're some other tasks on my plate for now and I couldn't focus on this.

As the review comments are not only pointing typo/syntax but also pointing the content as well, I may need to find enough block of time to go through. I'll try to find it in next week and update here. Thanks again!

SparkQA · 2020-02-16T05:25:14Z

Test build #118487 has finished for PR 27398 at commit 841d4d0.

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

HeartSaVioR · 2020-02-16T05:28:19Z

@dongjoon-hyun @tgravescs @gaborgsomogyi

I've reflected review comments and updated the PR description. As these comments are not only about typos or syntaxes, I guess my reflections may not be enough. Please take a second look and provide suggestions. Thanks in advance!

SparkQA · 2020-02-16T05:40:41Z

Test build #118488 has finished for PR 27398 at commit 803663f.

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

gaborgsomogyi · 2020-02-17T10:03:05Z

Coming back to this but just re-installing jekyll to test it...

gaborgsomogyi

LGTM from my side, only nits. (After some struggle I've installed the tools on my new machine)

gaborgsomogyi · 2020-02-17T17:44:12Z

docs/monitoring.md

+let you have rolling event log files instead of single huge event log file which may help some scenarios on its own,
+but it still doesn't help you reducing the overall size of logs.
+
+Spark History Server can apply 'compaction' on the rolling event log files to reduce the overall size of


Nit: Not sure what's the intention with this ‘ sign here. Maybe compaction word is not so special that it must be highlighted. This applies more places.

That's used as emphasizing "what" we are explaining here, but maybe not a big deal. Will remove.

gaborgsomogyi · 2020-02-17T17:49:39Z

docs/monitoring.md

+Once it selects the target, it analyzes them to figure out which events can be excluded, and rewrites them
+into one compact file with discarding events which are decided to exclude.
+
+The compaction tries to exclude the events which point to the outdated things like jobs, and so on. As of now, below describes


Nit: s/events which point to the outdated things like jobs/events which point to outdated data

SparkQA · 2020-02-17T22:56:17Z

Test build #118601 has finished for PR 27398 at commit a95a4a4.

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

HeartSaVioR · 2020-02-19T06:57:46Z

@dongjoon-hyun @tgravescs Kindly reminder.

tgravescs · 2020-02-24T19:57:31Z

thanks for pinging me will hopefully look later today or tomorrow

tgravescs

looks good, thanks @HeartSaVioR

HeartSaVioR · 2020-02-24T23:06:20Z

@dongjoon-hyun
Could you please help finalizing the review? I guess it might not be possible to get this reviewed by @vanzin in time. I'd be happy to follow your comment on lowering warn if you still think it's too aggressive.

dongjoon-hyun

+1, LGTM. Thank you all.
Merged to master/3.0.

…section of monitoring.md ### What changes were proposed in this pull request? This is a FOLLOW-UP PR for review comment on #27208 : #27208 (review) This PR documents a new feature `Eventlog Compaction` into the new section of `monitoring.md`, as it only has one configuration on the SHS side and it's hard to explain everything on the description on the single configuration. ### Why are the changes needed? Event log compaction lacks the documentation for what it is and how it helps. This PR will explain it. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Built docs via jekyll. > change on the new section <img width="951" alt="Screen Shot 2020-02-16 at 2 23 18 PM" src="https://user-images.githubusercontent.com/1317309/74599587-eb9efa80-50c7-11ea-942c-f7744268e40b.png"> > change on the table <img width="1126" alt="Screen Shot 2020-01-30 at 5 08 12 PM" src="https://user-images.githubusercontent.com/1317309/73431190-2e9c6680-4383-11ea-8ce0-815f10917ddd.png"> Closes #27398 from HeartSaVioR/SPARK-30481-FOLLOWUP-document-new-feature. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com> (cherry picked from commit 02f8165) Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

HeartSaVioR · 2020-02-26T00:14:47Z

Thanks all for reviewing and merging!

…section of monitoring.md ### What changes were proposed in this pull request? This is a FOLLOW-UP PR for review comment on apache#27208 : apache#27208 (review) This PR documents a new feature `Eventlog Compaction` into the new section of `monitoring.md`, as it only has one configuration on the SHS side and it's hard to explain everything on the description on the single configuration. ### Why are the changes needed? Event log compaction lacks the documentation for what it is and how it helps. This PR will explain it. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Built docs via jekyll. > change on the new section <img width="951" alt="Screen Shot 2020-02-16 at 2 23 18 PM" src="https://user-images.githubusercontent.com/1317309/74599587-eb9efa80-50c7-11ea-942c-f7744268e40b.png"> > change on the table <img width="1126" alt="Screen Shot 2020-01-30 at 5 08 12 PM" src="https://user-images.githubusercontent.com/1317309/73431190-2e9c6680-4383-11ea-8ce0-815f10917ddd.png"> Closes apache#27398 from HeartSaVioR/SPARK-30481-FOLLOWUP-document-new-feature. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

HeartSaVioR added 2 commits January 30, 2020 16:23

[SPARK-30481][CORE][FOLLOWUP] Document event log compaction into new …

49f51ff

…section of monitor.md

Add caution message as suggested from apache#27208 (comment)

90a3f82

dongjoon-hyun added SPARK CORE DOCUMENTATION labels Jan 30, 2020

dongjoon-hyun changed the title ~~[SPARK-30481][CORE][FOLLOWUP] Document event log compaction into new section of monitoring.md~~ [SPARK-30481][DOCS][FOLLOWUP] Document event log compaction into new section of monitoring.md Jan 30, 2020

dongjoon-hyun removed the SPARK CORE label Jan 30, 2020

dongjoon-hyun reviewed Jan 30, 2020

View reviewed changes

docs/monitoring.md Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jan 31, 2020

View reviewed changes

docs/monitoring.md Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jan 31, 2020

View reviewed changes

docs/monitoring.md Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jan 31, 2020

View reviewed changes

docs/monitoring.md Outdated Show resolved Hide resolved

reflect review comments partially

3e74e05

gaborgsomogyi reviewed Feb 3, 2020

View reviewed changes

tgravescs reviewed Feb 3, 2020

View reviewed changes

dongjoon-hyun added SPARK CORE DOCUMENTATION and removed DOCUMENTATION labels Feb 5, 2020

Reflect review comments

803663f

HeartSaVioR force-pushed the SPARK-30481-FOLLOWUP-document-new-feature branch from 841d4d0 to 803663f Compare February 16, 2020 05:29

gaborgsomogyi approved these changes Feb 17, 2020

View reviewed changes

Reflect review comments

a95a4a4

tgravescs approved these changes Feb 24, 2020

View reviewed changes

dongjoon-hyun approved these changes Feb 25, 2020

View reviewed changes

dongjoon-hyun closed this in 02f8165 Feb 25, 2020

HeartSaVioR deleted the SPARK-30481-FOLLOWUP-document-new-feature branch February 26, 2020 00:14


		but the details can be changed afterwards.

		Please note that Spark History Server may not compact the old event log files if figures out not a lot of space


		### Applying compaction of old event log files

		A long-running streaming application can bring a huge single event log file which may cost a lot to maintain and

[SPARK-30481][DOCS][FOLLOWUP] Document event log compaction into new section of monitoring.md #27398

[SPARK-30481][DOCS][FOLLOWUP] Document event log compaction into new section of monitoring.md #27398

Uh oh!

Conversation

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

Uh oh!

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!

SparkQA commented Jan 30, 2020

Uh oh!

SparkQA commented Jan 30, 2020

Uh oh!

HeartSaVioR commented Jan 30, 2020

Uh oh!

dongjoon-hyun commented Jan 30, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jan 31, 2020

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!

HeartSaVioR Feb 16, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Feb 7, 2020

Uh oh!

SparkQA commented Feb 16, 2020

Uh oh!

HeartSaVioR commented Feb 16, 2020

Uh oh!

SparkQA commented Feb 16, 2020

Uh oh!

gaborgsomogyi commented Feb 17, 2020

Uh oh!

gaborgsomogyi left a comment

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Jan 30, 2020 •

edited

Loading

HeartSaVioR Feb 16, 2020 •

edited

Loading

gaborgsomogyi Feb 17, 2020 •

edited

Loading