You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "Deependra-Patel (via GitHub)" <gi...@apache.org> on 2023/07/19 09:20:31 UTC
[GitHub] [spark] Deependra-Patel opened a new pull request, #42071: [SPARK-44209] Expose amount of shuffle data available on the node
Deependra-Patel opened a new pull request, #42071:
URL: https://github.com/apache/spark/pull/42071
This will be available as external shuffle service metric
### What changes were proposed in this pull request?
Adding three more metrics to ShuffleMetrics (exposed by External Shuffle Service) "totalShuffleDataBytes", "numAppsWithShuffleData" and "lastNodeShuffleMetricRefreshEpochMillis"
We add a new daemon that scans the disk every 30s (configurable) and sums up total size of the shuffle data.
### Why are the changes needed?
Adding these metrics would help in -
1. Deciding if we can decommission the node if no shuffle data present
2. Better live monitoring of customer's workload to see if there is skewed shuffle data present on the node
### Does this PR introduce _any_ user-facing change?
This documentation will need to be updated on next release:
https://spark.apache.org/docs/latest/monitoring.html#component-instance--shuffleservice
### How was this patch tested?
UTs are added
Also tested manually on a YARN cluster and metrics are getting published
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Deependra-Patel commented on pull request #42071: [SPARK-44209] Expose amount of shuffle data available on the node
Posted by "Deependra-Patel (via GitHub)" <gi...@apache.org>.
Deependra-Patel commented on PR #42071:
URL: https://github.com/apache/spark/pull/42071#issuecomment-1649543098
> This is going to be very expensive to compute, and has nontrivial performance impact (particularly on a service which
tends to be already loaded and critical).
We have been running 1 TB TPC-H/DS benchmarks daily with these changes and haven't seen any difference in performance. We are only listing the files and checking their length not actually reading the data itself, therefore shouldn't be that expense. Also, we do it every 30s.
> Exposing this from executors, as part of data gen and aggregating per node would be much more cheaper
Executors can be stopped when dynamic allocation is used, then we won't be able to read this metric from executor for aggregation. We will also have stale metric if ESS polls regularly before executor goes away. The scenario when there is no shuffle data on the node and get this metric value 0 is important to actually remove the node.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] melihsozdinler commented on a diff in pull request #42071: [SPARK-44209] Expose amount of shuffle data available on the node
Posted by "melihsozdinler (via GitHub)" <gi...@apache.org>.
melihsozdinler commented on code in PR #42071:
URL: https://github.com/apache/spark/pull/42071#discussion_r1297743479
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalBlockHandler.java:
##########
@@ -101,19 +113,46 @@ public ExternalShuffleBlockResolver getBlockResolver() {
public ExternalBlockHandler(
OneForOneStreamManager streamManager,
ExternalShuffleBlockResolver blockManager) {
- this(streamManager, blockManager, new NoOpMergedShuffleFileManager(null, null));
+ this(streamManager, blockManager, new NoOpMergedShuffleFileManager(null, null), -1);
}
/** Enables mocking out the StreamManager, BlockManager, and MergeManager. */
@VisibleForTesting
public ExternalBlockHandler(
OneForOneStreamManager streamManager,
ExternalShuffleBlockResolver blockManager,
- MergedShuffleFileManager mergeManager) {
+ MergedShuffleFileManager mergeManager,
+ int shuffleDataMetricRefreshPeriodSeconds) {
this.metrics = new ShuffleMetrics();
this.streamManager = streamManager;
this.blockManager = blockManager;
this.mergeManager = mergeManager;
+
+ if (shuffleDataMetricRefreshPeriodSeconds > 0) {
+ // We need daemon thread for shutdown hooks to be called properly
+ // This is also the reason we are not using ScheduledExecutorService
+ Runnable metricRefresherDaemon = () -> regularlyUpdateTotalShuffleDataMetric(
+ shuffleDataMetricRefreshPeriodSeconds);
+ new ThreadFactoryBuilder().setDaemon(true)
+ .setPriority(Thread.MIN_PRIORITY)
+ .setNameFormat("shuffle-data-metrics-refresher")
+ .build()
+ .newThread(metricRefresherDaemon)
+ .start();
+ }
+ }
+
+ private void regularlyUpdateTotalShuffleDataMetric(int refreshPeriod) {
+ while (true) {
+ try {
+ metrics.nodeShuffleMetrics =
+ new AtomicReference<>(blockManager.computeNodeShuffleMetrics());
+ Thread.sleep(1000L * refreshPeriod);
+ } catch (Exception e) {
+ // Catching exceptions so that subsequent executions are not suppressed
+ logger.debug("Exception occurred while calculating shuffle metrics", e);
Review Comment:
Better to use Warn or Error level here, since it is an exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #42071: [SPARK-44209] Expose amount of shuffle data available on the node
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #42071:
URL: https://github.com/apache/spark/pull/42071#issuecomment-1645080237
This is going to be very expensive to compute, and has nontrivial performance impact (particularly on a service which tends to be already loaded and critical).
Exposing this from executors, as part of data gen and aggregating per node would be much more cheaper
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44209] Expose amount of shuffle data available on the node [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #42071:
URL: https://github.com/apache/spark/pull/42071#issuecomment-1826450649
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] melihsozdinler commented on a diff in pull request #42071: [SPARK-44209] Expose amount of shuffle data available on the node
Posted by "melihsozdinler (via GitHub)" <gi...@apache.org>.
melihsozdinler commented on code in PR #42071:
URL: https://github.com/apache/spark/pull/42071#discussion_r1297748300
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalBlockHandler.java:
##########
@@ -70,6 +72,12 @@ public class ExternalBlockHandler extends RpcHandler
private static final String SHUFFLE_BLOCK_ID = "shuffle";
private static final String SHUFFLE_CHUNK_ID = "shuffleChunk";
+ static final String SHUFFLE_DATA_METRIC_REFRESH_INTERVAL_KEY =
+ "spark.shuffle.data.metric.refresh.interval.seconds";
Review Comment:
This config is also introduced in config/package.scala? How to configure this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-44209] Expose amount of shuffle data available on the node [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #42071: [SPARK-44209] Expose amount of shuffle data available on the node
URL: https://github.com/apache/spark/pull/42071
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org