You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by "leslizhang (via GitHub)" <gi...@apache.org> on 2024/03/19 09:48:09 UTC
[PR] Issue 1585 [incubator-uniffle]
leslizhang opened a new pull request, #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593
### What changes were proposed in this pull request?
added shuffle block size metric of type histogram.
### Why are the changes needed?
related feature https://github.com/apache/incubator-uniffle/issues/1585
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing UTs.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#discussion_r1582578270
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java:
##########
@@ -337,6 +349,9 @@ public void removeBuffer(String appId) {
removeBufferByShuffleId(appId, shuffleIdToBuffers.keySet());
shuffleSizeMap.remove(appId);
bufferPool.remove(appId);
+ if (appBlockSizeMetricEnabled) {
+ ShuffleServerMetrics.appHistogramWriteBlockSize.remove(appId);
Review Comment:
good practise.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#discussion_r1533204316
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java:
##########
@@ -131,6 +138,19 @@ public class ShuffleServerMetrics {
"topN_of_on_localfile_data_size_for_app";
public static final String TOPN_OF_ON_HADOOP_DATA_SIZE_FOR_APP =
"topN_of_on_hadoop_data_size_for_app";
+ public static final double[] BLOCK_SIZE_BUCKETS =
+ new double[] {
Review Comment:
Could you make this configurable like event_size_level?
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#discussion_r1533481471
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -316,6 +321,9 @@ public synchronized List<ShuffleBlockInfo> clear() {
+ dataSize
+ "], memoryUsed["
+ memoryUsed
+ + "],"
+ + "block number["
+ + result.size()
Review Comment:
I think `number of blocks` is easier to understand:
```suggestion
+ "], number of blocks["
+ result.size()
```
or simply `blocks`:
```suggestion
+ "], blocks["
+ result.size()
```
The term `block number` is too similar to `sequence number` in blockId.
##########
common/src/main/java/org/apache/uniffle/common/util/Constants.java:
##########
@@ -21,9 +21,12 @@ public final class Constants {
private Constants() {}
+ public static int _1kb = 1024;
+ public static int _1mb = 1024 * 1024;
Review Comment:
Why the underscore? Technically, these are `KiB` and `MiB`.
```suggestion
public static int KiB = 1024;
public static int MiB = 1024 * 1024;
```
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java:
##########
@@ -131,6 +138,19 @@ public class ShuffleServerMetrics {
"topN_of_on_localfile_data_size_for_app";
public static final String TOPN_OF_ON_HADOOP_DATA_SIZE_FOR_APP =
"topN_of_on_hadoop_data_size_for_app";
+ public static final double[] BLOCK_SIZE_BUCKETS =
+ new double[] {
+ 32 * _1kb,
+ 64 * _1kb,
+ 128 * _1kb,
+ 256 * _1kb,
+ 512 * _1kb,
+ _1mb,
+ 2 * _1mb,
+ 4 * _1mb,
+ 8 * _1mb,
+ 16 * _1mb
Review Comment:
Then this reads better:
```suggestion
32 * KiB,
64 * KiB,
128 * KiB,
256 * KiB,
512 * KiB,
1 * MiB,
2 * MiB,
4 * MiB,
8 * MiB,
16 * MiB
```
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#discussion_r1533407466
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -585,6 +585,12 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription(
"keep alive time of thread pool that used for calc summary metric, in SECONDS.");
+ public static final ConfigOption<Boolean> APP_LEVEL_SHUFFLE_BLOCK_SIZE_METRIC_ENABLED =
+ ConfigOptions.key("rss.server.block.size.metric.enabled")
Review Comment:
`rss.server.metrics.blockSizeStatisticsEnabled`
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#discussion_r1580717653
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -585,6 +585,12 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription(
"keep alive time of thread pool that used for calc summary metric, in SECONDS.");
+ public static final ConfigOption<Boolean> APP_LEVEL_SHUFFLE_BLOCK_SIZE_METRIC_ENABLED =
+ ConfigOptions.key("rss.server.block.size.metric.enabled")
Review Comment:
the name should reflect that this is per-app, not per server
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -572,6 +572,12 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription(
"keep alive time of thread pool that used for calc summary metric, in SECONDS.");
+ public static final ConfigOption<Boolean> APP_LEVEL_SHUFFLE_BLOCK_SIZE_METRIC_ENABLED =
+ ConfigOptions.key("rss.server.block.size.metric.enabled")
+ .booleanType()
+ .defaultValue(true)
Review Comment:
I think app-level metrics should not be enabled by default as this might overwhelm metric servers on large Spark clusters, especially if not used.
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java:
##########
@@ -131,6 +138,19 @@ public class ShuffleServerMetrics {
"topN_of_on_localfile_data_size_for_app";
public static final String TOPN_OF_ON_HADOOP_DATA_SIZE_FOR_APP =
"topN_of_on_hadoop_data_size_for_app";
+ public static final double[] BLOCK_SIZE_BUCKETS =
+ new double[] {
Review Comment:
You could provide the number of buckets (10), the min (32 * KiB), and a factor (2).
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java:
##########
@@ -712,7 +735,6 @@ public boolean limitHugePartition(
}
return false;
}
-
Review Comment:
looks unintended
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#issuecomment-2081979166
> LGTM. I will merge this. Thanks for your contribution
We should wait for @EnricoMi 's suggestion at least one or two days.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#issuecomment-2085047176
Thanks @EnricoMi @rickyma @zuston @leslizhang , merged to master.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#discussion_r1580748456
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -572,6 +572,20 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription(
"keep alive time of thread pool that used for calc summary metric, in SECONDS.");
+ public static final ConfigOption<Boolean> APP_LEVEL_SHUFFLE_BLOCK_SIZE_METRIC_ENABLED =
+ ConfigOptions.key("rss.server.metrics.blockSizeStatisticsEnabled")
+ .booleanType()
+ .defaultValue(false)
+ .withDescription("whether or not shuffle block size metric enabled");
+
+ public static final ConfigOption<String> APP_LEVEL_SHUFFLE_BLOCK_SIZE_METRIC_BUCKETS =
+ ConfigOptions.key("rss.server.metrics.blockSizeStatistics.buckets")
+ .stringType()
+ .defaultValue("32kb,64kb,128kb,256kb,512kb,1mb,2mb,4mb,8mb,16mb")
+ .withDescription(
+ "A comma-separated block size list, where each value"
+ + " can be suffixed with a memory size unit, such as kb ork, mb or m, etc.");
Review Comment:
kb or k
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] Issue 1585 [incubator-uniffle]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#issuecomment-2006956101
## [Codecov](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1593?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `77.41935%` with `7 lines` in your changes are missing coverage. Please review.
> Project coverage is 54.08%. Comparing base [(`403a7a9`)](https://app.codecov.io/gh/apache/incubator-uniffle/commit/403a7a956069a0b1965ec4b7914c4c3805d41190?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`dfcc53f`)](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1593?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1593?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...he/uniffle/server/buffer/ShuffleBufferManager.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9idWZmZXIvU2h1ZmZsZUJ1ZmZlck1hbmFnZXIuamF2YQ==) | 80.00% | [1 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...java/org/apache/uniffle/common/util/Constants.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi91dGlsL0NvbnN0YW50cy5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../apache/uniffle/common/metrics/MetricsManager.java](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9tZXRyaWNzL01ldHJpY3NNYW5hZ2VyLmphdmE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1593?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1593 +/- ##
============================================
+ Coverage 54.01% 54.08% +0.06%
+ Complexity 2863 2295 -568
============================================
Files 438 361 -77
Lines 24850 16125 -8725
Branches 2114 1471 -643
============================================
- Hits 13423 8721 -4702
+ Misses 10586 6896 -3690
+ Partials 841 508 -333
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-uniffle/pull/1593?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] Issue 1585 [incubator-uniffle]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#issuecomment-2006673833
## Test Results
2 307 files - 33 2 307 suites - 33 4h 19m 12s :stopwatch: - 12m 26s
909 tests + 1 905 :white_check_mark: - 2 1 :zzz: ±0 0 :x: ±0 3 :fire: + 3
10 493 runs - 48 10 468 :white_check_mark: - 59 14 :zzz: ±0 0 :x: ±0 11 :fire: +11
For more details on these errors, see [this check](https://github.com/apache/incubator-uniffle/runs/22826718574).
Results for commit 8537ab61. ± Comparison against base commit 403a7a95.
[test-results]:data:application/gzip;base64,H4sIAPlk+WUC/03NSw7CIBSF4a00jB3wuqV1MwYuNCH2YSiMjHv30kjt8HzAz5tNcQ47u3dScXPr2F5ivm5fks1xW0kEAEgiOs/1xsjHth57QTwI/vSMr/rqhMnGmYCfEFLaEokiSWWtTcH12GaLkvXDab+qbnCpHrtFRf0Yt2WJmRYbQBnreuGkRm5ckN6LyfWDcsE70NyjQRQO2OcLiXt/OhMBAAA=
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi merged PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] Issue 1585 [incubator-uniffle]
Posted by "zuston (via GitHub)" <gi...@apache.org>.
zuston commented on code in PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#discussion_r1533163326
##########
common/src/main/java/org/apache/uniffle/common/util/Constants.java:
##########
@@ -21,9 +21,12 @@ public final class Constants {
private Constants() {}
+ public static int _1kb = 1024;
+ public static int _1mb = 1024 * 1024;
// the value is used for client/server compatible, eg, online upgrade
public static final String SHUFFLE_SERVER_VERSION = "ss_v5";
public static final String METRICS_TAG_LABEL_NAME = "tags";
+ public static final String METRICS_APP_LABEL_NAME = "appid";
Review Comment:
appid -> appId
##########
client-spark/common/src/main/java/org/apache/spark/shuffle/writer/WriteBufferManager.java:
##########
@@ -179,6 +179,11 @@ public List<ShuffleBlockInfo> addPartitionData(
// check buffer size > spill threshold
if (usedBytes.get() - inSendListBytes.get() > spillSize) {
+ LOG.info(
Review Comment:
No need to use `string.format` . Just using the log.info("{}", xxxx)
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -585,6 +585,12 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription(
"keep alive time of thread pool that used for calc summary metric, in SECONDS.");
+ public static final ConfigOption<Boolean> APP_LEVEL_SHUFFLE_BLOCK_SIZE_METRIC_ENABLED =
+ ConfigOptions.key("rss.server.block.size.metric.enabled")
Review Comment:
How about `rss.server.blockSizeStatisticsMetrics.enabled`
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java:
##########
@@ -127,6 +134,9 @@ public ShuffleBufferManager(
this.hugePartitionMemoryLimitSize =
Math.round(
capacity * conf.get(ShuffleServerConf.HUGE_PARTITION_MEMORY_USAGE_LIMITATION_RATIO));
+ serverTags = initServerTags(conf);
Review Comment:
Why not using the tags initialized in `shuffleServer` directly?
##########
server/src/main/java/org/apache/uniffle/server/buffer/ShuffleBufferManager.java:
##########
@@ -707,4 +730,15 @@ public boolean limitHugePartition(
}
return false;
}
+
+ private String initServerTags(ShuffleServerConf conf) {
Review Comment:
remove 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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#discussion_r1533407466
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -585,6 +585,12 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription(
"keep alive time of thread pool that used for calc summary metric, in SECONDS.");
+ public static final ConfigOption<Boolean> APP_LEVEL_SHUFFLE_BLOCK_SIZE_METRIC_ENABLED =
+ ConfigOptions.key("rss.server.block.size.metric.enabled")
Review Comment:
`rss.server.blockSizeStatisticsMetricsEnabled`
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#discussion_r1533204316
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java:
##########
@@ -131,6 +138,19 @@ public class ShuffleServerMetrics {
"topN_of_on_localfile_data_size_for_app";
public static final String TOPN_OF_ON_HADOOP_DATA_SIZE_FOR_APP =
"topN_of_on_hadoop_data_size_for_app";
+ public static final double[] BLOCK_SIZE_BUCKETS =
+ new double[] {
Review Comment:
Could you make this configurable? Maybe you can provide the min, max and the number of buckets.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "jerqi (via GitHub)" <gi...@apache.org>.
jerqi commented on code in PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#discussion_r1533204316
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerMetrics.java:
##########
@@ -131,6 +138,19 @@ public class ShuffleServerMetrics {
"topN_of_on_localfile_data_size_for_app";
public static final String TOPN_OF_ON_HADOOP_DATA_SIZE_FOR_APP =
"topN_of_on_hadoop_data_size_for_app";
+ public static final double[] BLOCK_SIZE_BUCKETS =
+ new double[] {
Review Comment:
Could you make this configurable? Maybe you can provide the min, max.
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on code in PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#discussion_r1580752770
##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -572,6 +572,20 @@ public class ShuffleServerConf extends RssBaseConf {
.withDescription(
"keep alive time of thread pool that used for calc summary metric, in SECONDS.");
+ public static final ConfigOption<Boolean> APP_LEVEL_SHUFFLE_BLOCK_SIZE_METRIC_ENABLED =
+ ConfigOptions.key("rss.server.metrics.blockSizeStatisticsEnabled")
+ .booleanType()
+ .defaultValue(false)
+ .withDescription("whether or not shuffle block size metric enabled");
Review Comment:
Whether or not shuffle block size metric is enabled
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
Re: [PR] [#1585] feat(server): Support app-level block size statistics to report metrics [incubator-uniffle]
Posted by "rickyma (via GitHub)" <gi...@apache.org>.
rickyma commented on PR #1593:
URL: https://github.com/apache/incubator-uniffle/pull/1593#issuecomment-2081813240
ping~ @zuston @jerqi @EnricoMi
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org