[SPARK-29043][Core] Improve the concurrent performance of History Server by turboFei · Pull Request #25797 · apache/spark

turboFei · 2019-09-15T14:58:05Z

What changes were proposed in this pull request?

Even we set spark.history.fs.numReplayThreads to a large number, such as 30.
The history server still replays logs slowly.
We found that, if there is a straggler in a batch of replay tasks, all the other threads will wait for this
straggler.

In this PR, we create processing to save the logs which are being replayed.
So that the replay tasks can execute Asynchronously.

Why are the changes needed?

It can accelerate the speed to replay logs for history server.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UT.

dongjoon-hyun · 2019-09-15T20:24:18Z

ok to test

cc @wangyum

turboFei · 2019-09-15T22:58:35Z

retest please

HyukjinKwon · 2019-09-15T23:54:52Z

ok to test

HyukjinKwon · 2019-09-15T23:54:57Z

cc @vanzin too

HeartSaVioR

The code change looks OK, but I might be missing the effect where "listing" and "reloading applications" run concurrently. It would bring accessing some places via multi-threads which they weren't, so may need to check.

wangyum · 2019-09-16T04:23:02Z

ok to test

SparkQA · 2019-09-16T22:38:05Z

Test build #110636 has finished for PR 25797 at commit ec387f3.

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

turboFei · 2019-09-18T16:03:26Z

gentle ping @dongjoon-hyun @HyukjinKwon

kiszk · 2019-09-19T19:50:56Z

Looks good to me
Want to wait for comments from others cc @vanzin @wangyum

turboFei · 2019-09-20T02:33:19Z

I just modify the a method from protected to private in new commit, it would not impact the test result.

SparkQA · 2019-09-20T05:05:07Z

Test build #111038 has finished for PR 25797 at commit 1c36bfe.

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

Ngone51

Looks reasonable.

turboFei · 2019-09-20T14:29:41Z

I just add some comment and rename a method, which would not impact the test result.

SparkQA · 2019-09-20T16:58:44Z

Test build #111074 has finished for PR 25797 at commit 12f6300.

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

turboFei · 2019-09-26T03:24:32Z

@cloud-fan Could you help take a look?

kiszk · 2019-09-26T05:26:05Z

ping @vanzin @wangyum

gengliangwang · 2019-09-26T06:01:38Z

Seems good to me.
@turboFei Have you tested the changes manually?

turboFei · 2019-09-26T07:13:54Z

Seems good to me.
@turboFei Have you tested the changes manually?

I have tested it for a week, it works well.

vanzin · 2019-09-26T17:45:56Z

By unblocking the cleanForLogs call, you also unblock the cleaner task which may run after it. I didn't see any changes to that code; it should probably check whether some path is being processed before trying to do things with it.

turboFei · 2019-09-27T07:13:10Z

By unblocking the cleanForLogs call, you also unblock the cleaner task which may run after it. I didn't see any changes to that code; it should probably check whether some path is being processed before trying to do things with it.

Thanks, I will check it.

turboFei · 2019-09-27T08:01:11Z

By unblocking the cleanForLogs call, you also unblock the cleaner task which may run after it. I didn't see any changes to that code; it should probably check whether some path is being processed before trying to do things with it.

I have checked the code of cleanLogs, it will clean up the expired paths in listing and clear relative record in blackList.

In this PR, if a path is contained in processing, it would not be added into listing.

So, should we interrupt the tasks which is processing a path has been expired?

SparkQA · 2019-09-30T13:33:03Z

Test build #111606 has finished for PR 25797 at commit 55ad333.

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

vanzin · 2019-09-30T21:18:41Z

        .asScala
        .toList
-      stale.foreach { log =>
+      stale.filterNot(isProcessing(_)).foreach { log =>


filterNot(isProcessing)

But there's a problem here. Let's say you're unlucky and the cleaner task gets here for a log that should be cleanup up, but for some reason is still being processed by the tasks created by "checkForLogs".

Instead of cleaning that log, it will instead be skipped until the next time the cleaner task run, which may be quote a long time later.

yes, I think we do not need filter these logs which is contained in processing.

The scene that log is both in listing and processing would only happen when a log has been processed and written to the listing but the finally block(remove it from processing)
has not been executed.

But for this case, even if we cleanup these logs, it would has no impaction.
So, I think we don't need filter these logs here.
@vanzin

Is it safe if "cleaner task" and "replay task" access the same log directory concurrently? I guess we still need to filter out these logs for safety, but we need to also retry cleaning up these logs sooner, at least sooner than the interval of cleaner.

Sorry for late reply, I was on my National Day holiday for past several days.

Instead of cleaning that log, it will instead be skipped until the next time the cleaner task run, which may be quote a long time later.

but we need to also retry cleaning up these logs sooner, at least sooner than the interval of cleaner.

It sound a little complex, should we clean these logs with half interval of cleaner later?

@turboFei
Sorry I got lost on this and revisit this now.
I agree it could be complicated, as it's really just a cleaner. For workaround, we can just do two iterations in a single call, on first iteration try to do all, on second iteration try only with skipped logs.
@vanzin Would the workaround work for you? Or would we have to reschedule the interval?

I don't think that solves the problem. To solve that problem, the task that's processing the logs should check at the end whether the log file has expired.

Thanks, make sense.

SparkQA · 2019-11-06T16:54:20Z

Test build #113321 has finished for PR 25797 at commit 611ea30.

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

SparkQA · 2019-11-06T17:49:41Z

Test build #113328 has finished for PR 25797 at commit 832dd7a.

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

SparkQA · 2019-11-06T18:10:42Z

Test build #113330 has finished for PR 25797 at commit 8c0b532.

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

SparkQA · 2019-11-06T18:57:41Z

Test build #113332 has finished for PR 25797 at commit 66e9dd1.

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

vanzin · 2019-11-07T00:23:01Z

+      endProcessing(reader.rootPath)
+      pendingReplayTasksCount.decrementAndGet()
+
+      val isExpired = scanTime + conf.get(MAX_LOG_AGE_S) * 1000 < clock.getTimeMillis()


This is not right. Expiration is based on the time the log was last updated, not the time it was last scanned.

vanzin · 2019-11-07T00:26:24Z

+
+      val isExpired = scanTime + conf.get(MAX_LOG_AGE_S) * 1000 < clock.getTimeMillis()
+      if (isExpired) {
+        listing.delete(classOf[LogInfo], reader.rootPath.toString)


You may also need to remove the application attempt that refers to the log from the listing database.

Basically you have to do what cleanLogs does, both to define whether the log is expired, and what needs to be deleted.

SparkQA · 2019-11-07T12:13:29Z

Test build #113379 has finished for PR 25797 at commit d5ff72c.

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

turboFei · 2019-11-07T12:34:15Z

retest this please.

turboFei · 2019-11-12T10:30:05Z

rebased.

SparkQA · 2019-11-12T12:26:21Z

Test build #113625 has finished for PR 25797 at commit 579c1ad.

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

SparkQA · 2019-11-12T13:52:06Z

Test build #113626 has finished for PR 25797 at commit 7d1301c.

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

HeartSaVioR · 2019-12-13T05:07:51Z

@turboFei Hi, could you address the review comments? This is good to have and seems close to be merged (according to #26416 (review) ).

turboFei · 2019-12-13T06:46:54Z

@turboFei Hi, could you address the review comments? This is good to have and seems close to be merged (according to #26416 (review) ).

Thanks, I will address it as soon as possible.
Thanks for your reminder. @HeartSaVioR

turboFei · 2019-12-13T11:05:01Z

+
+    log.appId.foreach { appId =>
+      val app = listing.read(classOf[ApplicationInfoWrapper], appId)
+      if (app.oldestAttempt() <= maxTime) {


Here the logic is consistent with cleanLogs().
But, I think there is an overlap between app.oldestAttempt() <= maxTime and attempt.info.lastUpdated.getTime() >= maxTime, even it does not matter.

SparkQA · 2019-12-13T13:26:43Z

Test build #115300 has finished for PR 25797 at commit c6ac35e.

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

SparkQA · 2019-12-14T19:10:00Z

Test build #115334 has finished for PR 25797 at commit e9ebb6f.

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

vanzin

Looks ok now. Merging to master.

vanzin · 2019-12-16T22:38:54Z


+  test("SPARK-29043: clean up specified event log") {
+    val clock = new ManualClock()
+    val conf = createTestConf().set(MAX_LOG_AGE_S.key, "0").set(CLEANER_ENABLED.key, "true")


No need to use .key here.

(I'll fix this during merge.)

turboFei changed the title ~~[SPARK-29043] Improve the concurrent performance of History Server~~ [WIP][SPARK-29043] Improve the concurrent performance of History Server Sep 15, 2019

turboFei changed the title ~~[WIP][SPARK-29043] Improve the concurrent performance of History Server~~ [WIP][SPARK-29043][Core] Improve the concurrent performance of History Server Sep 15, 2019

turboFei changed the title ~~[WIP][SPARK-29043][Core] Improve the concurrent performance of History Server~~ [SPARK-29043][Core] Improve the concurrent performance of History Server Sep 15, 2019

HeartSaVioR reviewed Sep 16, 2019

View reviewed changes

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala Outdated

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala Outdated

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala

dongjoon-hyun added the SPARK CORE label Sep 16, 2019

kiszk reviewed Sep 16, 2019

View reviewed changes

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala Outdated

turboFei requested a review from kiszk September 17, 2019 03:05

Ngone51 reviewed Sep 20, 2019

View reviewed changes

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala Outdated

HeartSaVioR reviewed Sep 25, 2019

View reviewed changes

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala

gengliangwang approved these changes Sep 26, 2019

View reviewed changes

viirya reviewed Sep 27, 2019

View reviewed changes

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala Outdated

vanzin reviewed Sep 30, 2019

View reviewed changes

vanzin reviewed Nov 7, 2019

View reviewed changes

vanzin reviewed Nov 19, 2019

View reviewed changes

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala Outdated

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala Outdated

HeartSaVioR mentioned this pull request Dec 13, 2019

[SPARK-29779][CORE] Compact old event log files and cleanup #26416

Closed

turboFei added 6 commits December 13, 2019 14:47

[SPARK-29043] Improve the concurrent performance of History Server

b6cd802

check and delete expired log

7621626

remove used code

44e3c80

fix code

8540c39

add ut

34aeab8

fix style

c6ac35e

turboFei commented Dec 13, 2019

View reviewed changes

vanzin reviewed Dec 14, 2019

View reviewed changes

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala Outdated

Comment thread core/src/main/scala/org/apache/spark/deploy/history/FsHistoryProvider.scala

Comment thread core/src/test/scala/org/apache/spark/deploy/history/FsHistoryProviderSuite.scala

fix ut

e9ebb6f

vanzin reviewed Dec 16, 2019

View reviewed changes

vanzin closed this in 5954311 Dec 16, 2019

Conversation

turboFei commented Sep 15, 2019 • 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!

dongjoon-hyun commented Sep 15, 2019

Uh oh!

turboFei commented Sep 15, 2019

Uh oh!

HyukjinKwon commented Sep 15, 2019

Uh oh!

HyukjinKwon commented Sep 15, 2019

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wangyum commented Sep 16, 2019

Uh oh!

Uh oh!

SparkQA commented Sep 16, 2019

Uh oh!

turboFei commented Sep 18, 2019

Uh oh!

kiszk commented Sep 19, 2019

Uh oh!

turboFei commented Sep 20, 2019

Uh oh!

SparkQA commented Sep 20, 2019

Uh oh!

Ngone51 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

turboFei commented Sep 20, 2019

Uh oh!

SparkQA commented Sep 20, 2019

Uh oh!

Uh oh!

turboFei commented Sep 26, 2019

Uh oh!

kiszk commented Sep 26, 2019

Uh oh!

gengliangwang commented Sep 26, 2019

Uh oh!

turboFei commented Sep 26, 2019

Uh oh!

vanzin commented Sep 26, 2019

Uh oh!

turboFei commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

turboFei commented Sep 27, 2019

Uh oh!

Uh oh!

SparkQA commented Sep 30, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Oct 1, 2019 • 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!

turboFei Oct 11, 2019 • 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!

turboFei commented Sep 15, 2019 •

edited

Loading

turboFei commented Sep 27, 2019 •

edited

Loading

HeartSaVioR Oct 1, 2019 •

edited

Loading

turboFei Oct 11, 2019 •

edited

Loading

vanzin Dec 16, 2019 •

edited

Loading