[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20532: [SPARK-23353][CORE] Allow ExecutorMetricsUpdate e...
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-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[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 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-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
