You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by LantaoJin <gi...@git.apache.org> on 2018/02/07 13:45:59 UTC
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
GitHub user LantaoJin opened a pull request:
https://github.com/apache/spark/pull/20532
[SPARK-23353][CORE] Allow ExecutorMetricsUpdate events to be logged t…
…o the event log with sampling
## What changes were proposed in this pull request?
[SPARK-22050|https://issues.apache.org/jira/browse/SPARK-22050] give a way to log BlockUpdated events. Actually, the ExecutorMetricsUpdates are also very useful if it can be persisted for further analysis.
As a performance reason and actual use case, the PR offers a fraction configuration which can sample the events to be persisted. we also refactor for BlockUpdated with the same sampling way.
## How was this patch tested?
Unit tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/LantaoJin/spark SPARK-23353
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/20532.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #20532
----
commit 0b132b34c020594aee91386271576d715196ef21
Author: LantaoJin <ji...@...>
Date: 2018-02-07T13:42:18Z
[SPARK-23353][CORE] Allow ExecutorMetricsUpdate events to be logged to the event log with sampling
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166817939
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
Ok, actually greater than 1 has the same meaning with equals to 1 (100% selected)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166854856
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
"spark.eventLog.logVerboseEvent.enabled" sounds good. But this is hard to work in production environment. The risk is the evil in production environment. Sample with fraction 1 also equals to persist all events.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/20532
@jerryshao Could you have a time to help to review?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166852617
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
>how about control the max number of events recorded per time split?
I think this approach is still hard to balance the user requirement and event log size. Spark will possibly ignore the events that is required by the user at the specific time.
IMO, using "true" or "false" might be a feasible solution - whether to dump all the events or just ignore them. For normal user, by default (false) should be enough for them, but if you want further analysis, you can enable this by taking the risk of large event file.
For the configuration, I think we could use something like "spark.eventLog.logVerboseEvent.enabled" to control all the verbose events that will be dumped manually.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/20532
Thanks everyone. So just close it? Or easily leave an enabled switch like blockUpdated dose? I am all OK.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on the issue:
https://github.com/apache/spark/pull/20532
I would suggest to do it like what we have already done for block update event. Since we already opened a door for block update event, it is also acceptable to leave room for another event. User should be responsible for the big event log file.
@jiangxb1987 what is your opinion?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166833429
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
It is actually hard for users to set a sample ratio to balance the event log size and analysis requirement, how about control the max number of events recorded per time split?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/20532
I'm still wondering whether event log is supposed to work this way, that as the source for customized analysis. I really feel we shall need some event/metrics logging framework that serves for external analysis, but we really need investigate into more use cases and give a more thoughtful design on that issue.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166805463
--- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
@@ -228,14 +231,23 @@ private[spark] class EventLoggingListener(
logEvent(event, flushLogger = true)
}
+ // Only sampled logging when fraction greater than zero, fraction equals 1 means logging all
override def onBlockUpdated(event: SparkListenerBlockUpdated): Unit = {
- if (shouldLogBlockUpdates) {
- logEvent(event, flushLogger = true)
+ if (blockUpdateSampleFraction > 0) {
--- End diff --
Since you already checked this configuration, it is not necessary to check here again.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166818413
--- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
@@ -228,14 +231,23 @@ private[spark] class EventLoggingListener(
logEvent(event, flushLogger = true)
}
+ // Only sampled logging when fraction greater than zero, fraction equals 1 means logging all
override def onBlockUpdated(event: SparkListenerBlockUpdated): Unit = {
- if (shouldLogBlockUpdates) {
- logEvent(event, flushLogger = true)
+ if (blockUpdateSampleFraction > 0) {
--- End diff --
Think it again. Seems check `>0` could avoid generate a random number when it is configured to 0.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20532
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20532
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166860784
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
Yes. I know a custom SparkListener can do the dump. But actually we can dump to eventlog file. So the conclusion is that sample is unacceptable? Is adding a configuration like "spark.eventLog.logVerboseEvent.enabled" to enable it enough?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166862037
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
I'm inclined to "spark.eventLog.logVerboseEvent.enabled". But I don't have a strong preference, maybe other reviewers have different thoughts about it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166844625
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
Agree, I think a max limitation is necessary.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166805197
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
+ .createWithDefault(0.0)
+
+ private[spark] val EVENT_LOG_EXECUTOR_METRICS_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logExecutorMetricsUpdates.fraction")
+ .doc("Expected number of times each executorMetricsUpdate event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
ditto.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on the issue:
https://github.com/apache/spark/pull/20532
I agree with @jiangxb1987 . @LantaoJin would you please elaborate the usage scenario of dumping executor metrics to event log? Seems history server doesn't leverage such information necessarily.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166817978
--- Diff: core/src/main/scala/org/apache/spark/scheduler/EventLoggingListener.scala ---
@@ -228,14 +231,23 @@ private[spark] class EventLoggingListener(
logEvent(event, flushLogger = true)
}
+ // Only sampled logging when fraction greater than zero, fraction equals 1 means logging all
override def onBlockUpdated(event: SparkListenerBlockUpdated): Unit = {
- if (shouldLogBlockUpdates) {
- logEvent(event, flushLogger = true)
+ if (blockUpdateSampleFraction > 0) {
--- End diff --
sure
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166855772
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
But if you're using sampling, you will possibly miss some events, for example if you're tracking memory usage, you may miss the peak memory usage events that might be important to your analysis.
Ideally, I think you can write a custom SparkListener to dump executor metrics to some time series DB like openTSDB, which might be better compared to analysis the static files.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/20532
I agree with @jiangxb1987 ... we already have issues with event logs being too big, as it the driver gets backlogged even writing them out, and then the history server takes a long time to parse those files. There have been recent improvements to that, but doesn't mean we should reintroduce the problem.
I'm not saying this doesn't have a use, I'd just like to figure out if this the best way to do it. If it only has one very specific use case for @LantaoJin , then maybe they have an alternative still using public apis, with a custom listener as I suggested. I worry a user might turn this on (why not, more data is better) and then later on hit other scaling challenges and not realize this was the problem.
Or if this does have some general use case for all users, then maybe its fine, but I haven't seen that yet. And maybe there is a better way to do that ... do we need another way to get detailed output metrics from executors, that doesn't have some of the scaling challenges of the event log?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166805138
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
Should you also check if the configuration is `<= 1.0`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/20532
I'm also worried that if we want to sample more events in the future, we have to add more configs following this way, which doesn't sound like a perfect choice.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166864086
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
Thank you sir. Will wait more comments.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/20532#discussion_r166855350
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -53,10 +53,21 @@ package object config {
.booleanConf
.createWithDefault(false)
- private[spark] val EVENT_LOG_BLOCK_UPDATES =
- ConfigBuilder("spark.eventLog.logBlockUpdates.enabled")
- .booleanConf
- .createWithDefault(false)
+ private[spark] val EVENT_LOG_BLOCK_UPDATES_FRACTION =
+ ConfigBuilder("spark.eventLog.logBlockUpdates.fraction")
+ .doc("Expected number of times each blockUpdated event is chosen to log, " +
+ "fraction must be [0, 1]. 0 by default, means disabled")
+ .doubleConf
+ .checkValue(_ >= 0, "The fraction must not be negative")
--- End diff --
It is not only for debugging, but also for deploying to all applications. So we need a safe way to do that. That's max limitation seems much acceptable than just enabled or disabled.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/20532
Emmm... in case we want to sample more events, does that means we shall add a new config for each event sampling?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/20532
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/20532
Thanks @jerryshao. Changed to simply add a new configuration instead sampling logging.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/20532
@jerryshao and @jiangxb1987 , thanks for your advice. In 2.1.x, the two Update events (BlockUpdated & ExecutorMetricsUpdate) are all dumb. And in 2.2.x, only BlockUpdated event has a configuration to be logged. So the fair way is just adding an enable configuration to logging ExecutorMetrics for further using. But actually, we are refactoring the heartbeat to report more Executor information to the Driver. And this information will be logged to event log to be analysed. You know the Update events are so mass and they are not sequential. So we don't need all the events to do analysing. For example, we report Heap Usage after GC of all Executors, 10% events also can help us to get the result we want. So just add a configuration "enabled" for ExecutorMetricsUpdate is poorly simple in a big production cluster. BTW, the work I mentioned about will be contributed back to community as well soon.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate events t...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/20532
I can see why you want this sometimes, but I'm trying to figure out if its really valuable for users in general. You could always add a custom listener to log this info. It would go into separate file, not the std event log file, which means you'd have a little more work to do to stitch them together. OTOH that could be a good thing, as it means these history server wouldn't have to parse those extra lines.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org