[SPARK-20923] turn tracking of TaskMetrics._updatedBlockStatuses off #18162

tgravescs · 2017-05-31T14:46:47Z

What changes were proposed in this pull request?

Turn tracking of TaskMetrics._updatedBlockStatuses off by default. As far as I can see its not used by anything and it uses a lot of memory when caching and processing a lot of blocks. In my case it was taking 5GB of a 10GB heap and I even went up to 50GB heap and the job still ran out of memory. With this change in place the same job easily runs in less then 10GB of heap.

We leave the api there as well as a config to turn it back on just in case anyone is using it. TaskMetrics is exposed via SparkListenerTaskEnd so if users are relying on it they can turn it back on.

How was this patch tested?

Ran unit tests that were modified and manually tested on a couple of jobs (with and without caching). Clicked through the UI and didn't see anything missing.
Ran my very large hive query job with 200,000 small tasks, 1000 executors, cached 6+TB of data this runs fine now whereas without this change it would go into full gcs and eventually die.

tgravescs · 2017-05-31T14:47:48Z

@JoshRosen

JoshRosen · 2017-05-31T15:39:47Z

core/src/main/scala/org/apache/spark/storage/BlockManager.scala

I'm having flashbacks to how tricky it was to figure out this logic when refactoring the block manager code (this used to be a lot messier). Really happy to see this get removed.

JoshRosen · 2017-05-31T15:42:30Z

core/src/main/scala/org/apache/spark/util/JsonProtocol.scala

The only thing that maybe gives me pause here is compatibility when reading Spark logs produced by a new version of Spark in an old version of the History Server: if we remove a key which was present from day 0 then we might run into problems when code assumes it will be present. That said, it looks like the read of this key down on line 842 was already using Utils.jsonOption so maybe this was added later and thus is already handled as an option in the existing History Server code.

Good point, I'll look through some older versions and test out a few things with this.

If we aren't sure I can also just leave it here but let it be empty

I looked at this some more and I don't see any issue with this. As you said below on 842 its an option and I tested this both ways with history server as well just to be sure. new history file (without "Updated blocks" entry) with old history server and old file (with "Updated blocks" entry in history file) with new history server, both work fine.

if you think we should leave it I can put it back with a empty value?

cloud-fan · 2017-05-31T17:18:05Z

core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala

Unfortunately TaskMetrics is a public class(via SparkListenerTaskEnd), so this is a breaking change.

But I do agree we should not track the updated block status, how about we still keep this method and make it always return Nil? We can mark it as deprecated and add comments to say that it's only here for binary compatibility.

yeah I'll update it

SparkQA · 2017-05-31T17:34:25Z

Test build #77594 has finished for PR 18162 at commit 5327ba5.

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

JoshRosen · 2017-05-31T17:37:54Z

@cloud-fan has a good point about backwards-compatibility. In case any folks are actually relying on this behavior, I wonder whether we could mark it as deprecated and have a flag for disabling it instead of complete removal.

tgravescs · 2017-05-31T19:01:08Z

Updated, I put the TaskMetrics api back with deprecated marking and just had it return Nil. @JoshRosen Were you thinking of adding more back?

JoshRosen · 2017-05-31T20:40:34Z

@tgravescs, I guess the question is whether any user has a SparkListener which actually uses the value of updatedBlockStatuses for some monitoring application or something similar. In that case returning Nil will preserve binary compatibility but will break semantics for their app. I don't know of any use-cases like this offhand so I don't have a personal stake in this, but I could imagine problems in case someone relies on the return value (hence suggestion of flagging if we want to be really conservative).

tgravescs · 2017-05-31T21:35:31Z

taskMetrics doesn't take the sparkconf or anything to get at a config so we would have to config out everywhere its incrementing or adding things. I think that wouldn't be to hard. I'll put everything back and just add config around them with default off.

SparkQA · 2017-05-31T21:50:04Z

Test build #77603 has finished for PR 18162 at commit 41ed774.

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

tgravescs · 2017-06-01T04:15:43Z

core/src/main/scala/org/apache/spark/internal/config/package.scala

      .createWithDefaultString("200m")
+
+  private[spark] val TASK_METRICS_TRACK_UPDATED_BLOCK_STATUSES =
+    ConfigBuilder("spark.taskMetrics.trackUpdatedBlockStatuses")


Not sure if we want to document this somewhere? If so what do you suggest there isn't any other config quite like this right now. Could either put in configuration doc or monitoring doc.

you can document it in configuration.md.

Actually can we turn it off by default? I think this is feature is useless for most of the users. cc @JoshRosen

Right this is why I originally had this off by default and you requested it on. I will turn it back on and if a user finds they need it because they somehow extended the class they can turn it on. Turning this off will be most beneficial for the majority of users

cloud-fan · 2017-06-01T06:05:23Z

core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala

+   * By default it is configured to not actually save the block statuses via config
+   * TASK_METRICS_TRACK_UPDATED_BLOCK_STATUSES.
   */
  def updatedBlockStatuses: Seq[(BlockId, BlockStatus)] = {


Shall we deprecate it? Considering the cost of collecting this information, I think we should not encourage users to use it...

cloud-fan · 2017-06-01T06:09:52Z

I do love the clean up you did by removing updatedBlockStatuses entirely... Since SparkListenerTaskEnd is marked as a developer API, is it acceptable to make TaskMetrics.updatedBlockStatuses always return Nil in Spark 2.3? Maybe we can send an email to dev list to ask about this.

cc @srowen

SparkQA · 2017-06-01T07:02:04Z

Test build #77625 has finished for PR 18162 at commit ec3c29d.

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

tgravescs · 2017-06-01T13:30:59Z

Yeah I was figuring I would file another jira to remove it later. I can add the deprecated flag here if you guys agree.

tgravescs · 2017-06-01T13:33:04Z

It would be nice to get this into spark 2.2 if we can

tgravescs · 2017-06-01T20:09:09Z

@JoshRosen what do you think should we add the deprecated?

cloud-fan · 2017-06-01T20:49:38Z

Yea I think we should merge this to 2.2, but we need to change the default value of the new config to true, to not surprise users.

tgravescs · 2017-06-02T19:18:46Z

ok, I'll update the default.

tgravescs · 2017-06-02T19:24:06Z

turned on by default for backwards compatibility but don't really agree with it. We should make it more stable/usable for people by turning it off. I'm assuming anyone that is using this would be very minority of people, but to be safe we can leave it on and I'll file a separate jira to turn off and deprecate. Note I'm out of office next week so if comments I might not respond for a bit.

SparkQA · 2017-06-02T19:24:24Z

Test build #77682 has finished for PR 18162 at commit fbe30cf.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-06-02T19:50:05Z

core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala

remove this line as it's not corrected now.

cloud-fan · 2017-06-02T19:56:22Z

core/src/main/scala/org/apache/spark/ui/jobs/JobProgressListener.scala

When we reach here, I think we already stored updatedBlockStatus in memory, filtering them out here doesn't help.

I'm not sure what you mean by its already stored? It gets stored into the TaskMetrics when the call below to TaskMetrics.fromAccumulatorInfo is made, now the task metrics UpdatedBlockStatuses it returns aren't really ever used by this function in updateAggregateMetrics or updateTaskMetric., but I didn't see any reason to set it since its not used.

the accumUpdates was sent from executors, so if we already stopped tracking updatedBlockStatus, we don't need to do filter here.

ping @tgravescs , any ideas? If it's not doable, we can start a voting and decide whether to remove updatedBlockStatus entirely.

to be more clear, I think we should just do an assert here to make sure there is no UPDATED_BLOCK_STATUSES accumulator updates, instead of doing a filter.

yes that is correct as I said it shouldn't really ever get there so I can add an assert. Sorry for the delay was out of office for a while. I'll update it.

How about I just revert this. I'm not sure its worth an assert here. the updated block statuses are just going to be empty so setting it to empty isn't going to hurt anything.

that also works

cloud-fan · 2017-06-02T19:57:45Z

I'm wondering if it's doable to stop tracking the updatedBlockStatus according to a config... there are many places that update updatedBlockStatus right?

tgravescs · 2017-06-02T20:21:41Z

I'm not sure what you mean by its not doable? what places are you seeing update the block statuses that I haven't covered here? most of it was done by the BlockManager. Maybe I'm missing something in my intellij search or greps but I dont' think so. Look to see where incUpdatedBlockStatuses, setUpdatedBlockStatuses (2 versions) are used.

SparkQA · 2017-06-02T21:38:40Z

Test build #77684 has finished for PR 18162 at commit 018955a.

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

SparkQA · 2017-06-02T22:40:14Z

Test build #77687 has finished for PR 18162 at commit a875aac.

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

rdblue · 2017-06-08T21:28:21Z

@tgravescs, I deployed this to our production environment (based on 2.0.0) a few days ago and haven't hit any problems with it. I think this is good to go, unless something has been added recently that uses the block statuses.

+1

tgravescs · 2017-06-20T17:05:10Z

Jenkins, test this please

SparkQA · 2017-06-20T19:09:03Z

Test build #78300 has finished for PR 18162 at commit a875aac.

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

cloud-fan · 2017-06-21T01:07:16Z

@tgravescs can you address #18162 (comment) ?

tgravescs · 2017-06-21T14:21:17Z

sorry missed that you had commented, yes we can change that

…by default

tgravescs · 2017-06-22T16:49:49Z

will upmerge shortly, since there are conflicts

tgravescs · 2017-06-22T17:45:12Z

upmerged to master and updated default and removed unneeded changes.

SparkQA · 2017-06-22T17:52:09Z

Test build #78466 has finished for PR 18162 at commit 68fbd9f.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2017-06-22T17:58:25Z

failure is from previous push of code.

tgravescs · 2017-06-22T17:58:30Z

Jenkins, test this please

SparkQA · 2017-06-22T18:07:02Z

Test build #78467 has finished for PR 18162 at commit 68fbd9f.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-06-22T18:54:40Z

Test build #78460 has finished for PR 18162 at commit c9bf058.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

tgravescs · 2017-06-22T20:33:07Z

Jenkins, test this please

SparkQA · 2017-06-22T21:01:49Z

Test build #78468 has finished for PR 18162 at commit 289e993.

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

cloud-fan · 2017-06-23T01:19:30Z

thanks, merging to master!

tgravescs · 2017-06-23T14:02:43Z

thanks for the reviews @cloud-fan

## What changes were proposed in this pull request? Turn tracking of TaskMetrics._updatedBlockStatuses off by default. As far as I can see its not used by anything and it uses a lot of memory when caching and processing a lot of blocks. In my case it was taking 5GB of a 10GB heap and I even went up to 50GB heap and the job still ran out of memory. With this change in place the same job easily runs in less then 10GB of heap. We leave the api there as well as a config to turn it back on just in case anyone is using it. TaskMetrics is exposed via SparkListenerTaskEnd so if users are relying on it they can turn it back on. ## How was this patch tested? Ran unit tests that were modified and manually tested on a couple of jobs (with and without caching). Clicked through the UI and didn't see anything missing. Ran my very large hive query job with 200,000 small tasks, 1000 executors, cached 6+TB of data this runs fine now whereas without this change it would go into full gcs and eventually die. Author: Thomas Graves <tgraves@thirteenroutine.corp.gq1.yahoo.com> Author: Tom Graves <tgraves@yahoo-inc.com> Closes apache#18162 from tgravescs/SPARK-20923.

## What changes were proposed in this pull request? Turn tracking of TaskMetrics._updatedBlockStatuses off by default. As far as I can see its not used by anything and it uses a lot of memory when caching and processing a lot of blocks. In my case it was taking 5GB of a 10GB heap and I even went up to 50GB heap and the job still ran out of memory. With this change in place the same job easily runs in less then 10GB of heap. We leave the api there as well as a config to turn it back on just in case anyone is using it. TaskMetrics is exposed via SparkListenerTaskEnd so if users are relying on it they can turn it back on. ## How was this patch tested? Ran unit tests that were modified and manually tested on a couple of jobs (with and without caching). Clicked through the UI and didn't see anything missing. Ran my very large hive query job with 200,000 small tasks, 1000 executors, cached 6+TB of data this runs fine now whereas without this change it would go into full gcs and eventually die. Author: Thomas Graves <tgraves@thirteenroutine.corp.gq1.yahoo.com> Author: Tom Graves <tgraves@yahoo-inc.com> Closes apache#18162 from tgravescs/SPARK-20923. (cherry picked from commit 5b5a69b)

Turn tracking of TaskMetrics._updatedBlockStatuses off by default. As far as I can see its not used by anything and it uses a lot of memory when caching and processing a lot of blocks. In my case it was taking 5GB of a 10GB heap and I even went up to 50GB heap and the job still ran out of memory. With this change in place the same job easily runs in less then 10GB of heap. We leave the api there as well as a config to turn it back on just in case anyone is using it. TaskMetrics is exposed via SparkListenerTaskEnd so if users are relying on it they can turn it back on. Ran unit tests that were modified and manually tested on a couple of jobs (with and without caching). Clicked through the UI and didn't see anything missing. Ran my very large hive query job with 200,000 small tasks, 1000 executors, cached 6+TB of data this runs fine now whereas without this change it would go into full gcs and eventually die. Author: Thomas Graves <tgraves@thirteenroutine.corp.gq1.yahoo.com> Author: Tom Graves <tgraves@yahoo-inc.com> Closes apache#18162 from tgravescs/SPARK-20923.

JoshRosen reviewed May 31, 2017

View reviewed changes

cloud-fan reviewed May 31, 2017

View reviewed changes

tgravescs force-pushed the SPARK-20923 branch from 41ed774 to ec3c29d Compare June 1, 2017 04:12

tgravescs changed the title ~~[SPARK-20923] Remove TaskMetrics._updatedBlockStatuses~~ [SPARK-20923] turn tracking of TaskMetrics._updatedBlockStatuses off Jun 1, 2017

tgravescs commented Jun 1, 2017

View reviewed changes

cloud-fan reviewed Jun 1, 2017

View reviewed changes

cloud-fan reviewed Jun 2, 2017

View reviewed changes

core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala Outdated

Copy link

Contributor

cloud-fan Jun 2, 2017

Choose a reason for hiding this comment

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

remove this line as it's not corrected now.

cloud-fan reviewed Jun 2, 2017

View reviewed changes

Thomas Graves and others added 6 commits June 22, 2017 15:42

[SPARK-20923] turn tracking of TaskMetrics._updatedBlockStatuses off …

7f418bb

…by default

Add tests

ae26d40

remove logging

ce0b51e

Turn on by default

d66d87c

Remove unneeded changes in JobProgressListener

c485cb6

Make default false and update test

f949f07

tgravescs force-pushed the SPARK-20923 branch from c9bf058 to f949f07 Compare June 22, 2017 17:42

update docs

68fbd9f

fix config reference from upmerge

289e993

asfgit closed this in 5b5a69b Jun 23, 2017

superbobry mentioned this pull request Dec 16, 2017

[SPARK-22805][CORE] Use StorageLevel aliases in event logs #19992

Closed

MikhailErofeev mentioned this pull request Dec 17, 2017

[SPARK-20923] turn tracking of TaskMetrics._updatedBlockStatuses off criteo-forks/spark#68

Merged

[SPARK-20923] turn tracking of TaskMetrics._updatedBlockStatuses off #18162

[SPARK-20923] turn tracking of TaskMetrics._updatedBlockStatuses off #18162

Uh oh!

Conversation

tgravescs commented May 31, 2017 • 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!

tgravescs commented May 31, 2017

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 May 31, 2017

Uh oh!

JoshRosen commented May 31, 2017

Uh oh!

tgravescs commented May 31, 2017

Uh oh!

JoshRosen commented May 31, 2017

Uh oh!

tgravescs commented May 31, 2017

Uh oh!

SparkQA commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jun 22, 2017 • 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!

cloud-fan commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jun 1, 2017

Uh oh!

tgravescs commented Jun 1, 2017

Uh oh!

tgravescs commented Jun 1, 2017

Uh oh!

tgravescs commented Jun 1, 2017

Uh oh!

cloud-fan commented Jun 1, 2017

Uh oh!

tgravescs commented Jun 2, 2017

Uh oh!

tgravescs commented Jun 2, 2017

Uh oh!

SparkQA commented Jun 2, 2017

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!

cloud-fan Jun 16, 2017 • 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!

tgravescs commented May 31, 2017 •

edited

Loading

cloud-fan Jun 22, 2017 •

edited

Loading

cloud-fan commented Jun 1, 2017 •

edited

Loading

cloud-fan Jun 16, 2017 •

edited

Loading