You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "turboFei (via GitHub)" <gi...@apache.org> on 2024/10/30 22:55:02 UTC
[PR] Record fallback [celeborn]
turboFei opened a new pull request, #2866:
URL: https://github.com/apache/celeborn/pull/2866
<!--
Thanks for sending a pull request! Here are some tips for you:
- Make sure the PR title start w/ a JIRA ticket, e.g. '[CELEBORN-XXXX] Your PR title ...'.
- Be sure to keep the PR description updated to reflect all changes.
- Please write your PR title to summarize what this PR proposes.
- If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
### Why are the changes needed?
### Does this PR introduce _any_ user-facing change?
### How was this patch tested?
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1830255700
##########
common/src/main/scala/org/apache/celeborn/common/metrics/source/AbstractSource.scala:
##########
@@ -321,6 +358,22 @@ abstract class AbstractSource(conf: CelebornConf, role: String)
updateInnerMetrics(sb.toString())
}
+ def recordMeter(nm: NamedMeter): Unit = {
+ val timestamp = System.currentTimeMillis
+ val sb = new StringBuilder
+ val label = nm.labelString
+ sb.append(s"${normalizeKey(nm.name)}Count$label ${nm.meter.getCount} $timestamp\n")
+ sb.append(s"${normalizeKey(nm.name)}MeanRate$label ${nm.meter.getMeanRate} $timestamp\n")
+ sb.append(
+ s"${normalizeKey(nm.name)}OneMinuteRate$label ${nm.meter.getOneMinuteRate} $timestamp\n")
+ sb.append(
+ s"${normalizeKey(nm.name)}FiveMinuteRate$label ${nm.meter.getFiveMinuteRate} $timestamp\n")
+ sb.append(
+ s"${normalizeKey(nm.name)}FifteenMinuteRate$label ${nm.meter.getFifteenMinuteRate} $timestamp\n")
Review Comment:
<img width="1188" alt="image" src="https://github.com/user-attachments/assets/8193c12c-5dc9-4783-b64b-6a8449a1bea4">
cc @FMX
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825635333
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
According to the Java doc, I think the LongAdder handled the resizing of the cells arrays. Is there any evidence to tell that the cells array in LongAdder will increase without limitation?
```
/**
* Handles cases of updates involving initialization, resizing,
* creating new Cells, and/or contention. See above for
* explanation. This method suffers the usual non-modularity
* problems of optimistic retry code, relying on rechecked sets of
* reads.
*
* @param x the value
* @param fn the update function, or null for add (this convention
* avoids the need for an extra field or function in LongAdder).
* @param wasUncontended false if CAS failed before call
*/
final void longAccumulate(long x, LongBinaryOperator fn,
boolean wasUncontended) {
```
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1831984192
##########
common/src/main/scala/org/apache/celeborn/common/metrics/source/AbstractSource.scala:
##########
@@ -321,6 +358,22 @@ abstract class AbstractSource(conf: CelebornConf, role: String)
updateInnerMetrics(sb.toString())
}
+ def recordMeter(nm: NamedMeter): Unit = {
+ val timestamp = System.currentTimeMillis
+ val sb = new StringBuilder
+ val label = nm.labelString
+ sb.append(s"${normalizeKey(nm.name)}Count$label ${nm.meter.getCount} $timestamp\n")
+ sb.append(s"${normalizeKey(nm.name)}MeanRate$label ${nm.meter.getMeanRate} $timestamp\n")
+ sb.append(
+ s"${normalizeKey(nm.name)}OneMinuteRate$label ${nm.meter.getOneMinuteRate} $timestamp\n")
+ sb.append(
+ s"${normalizeKey(nm.name)}FiveMinuteRate$label ${nm.meter.getFiveMinuteRate} $timestamp\n")
+ sb.append(
+ s"${normalizeKey(nm.name)}FifteenMinuteRate$label ${nm.meter.getFifteenMinuteRate} $timestamp\n")
Review Comment:
Finally, we have meters. That's good news.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#issuecomment-2461212207
> > because the fallback itself is a somewhat random occurrence
>
> Hi @RexXiong , I am fine for this, will just change the metrics type and reserve the code for Meter metrics in case we need it in the future.
Okay, dashboard.json need change too.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX closed pull request #2866: [CELEBORN-1680] Introduce ShuffleFallbackCount metrics
URL: https://github.com/apache/celeborn/pull/2866
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825492906
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
I am worried that the the LongAdder maintain an array for the added Long numbers.
And it might be heavy to the sum of this array if we always add element into it and does not make aggregation.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1830252740
##########
common/src/main/scala/org/apache/celeborn/common/metrics/source/AbstractSource.scala:
##########
@@ -321,6 +358,22 @@ abstract class AbstractSource(conf: CelebornConf, role: String)
updateInnerMetrics(sb.toString())
}
+ def recordMeter(nm: NamedMeter): Unit = {
+ val timestamp = System.currentTimeMillis
+ val sb = new StringBuilder
+ val label = nm.labelString
+ sb.append(s"${normalizeKey(nm.name)}Count$label ${nm.meter.getCount} $timestamp\n")
+ sb.append(s"${normalizeKey(nm.name)}MeanRate$label ${nm.meter.getMeanRate} $timestamp\n")
+ sb.append(
+ s"${normalizeKey(nm.name)}OneMinuteRate$label ${nm.meter.getOneMinuteRate} $timestamp\n")
+ sb.append(
+ s"${normalizeKey(nm.name)}FiveMinuteRate$label ${nm.meter.getFiveMinuteRate} $timestamp\n")
+ sb.append(
+ s"${normalizeKey(nm.name)}FifteenMinuteRate$label ${nm.meter.getFifteenMinuteRate} $timestamp\n")
Review Comment:
Refer: https://github.com/apache/spark/blob/667cff76b248c5ea00dd2be2fa9791224053c452/core/src/main/scala/org/apache/spark/metrics/sink/PrometheusServlet.scala#L93-L101
<img width="781" alt="image" src="https://github.com/user-attachments/assets/73e93a1b-31a1-44d7-b6d0-6c65190d3da3">
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825632292
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
After checking the source code of JDK, I found your intention to call `sumThenReset` is invalid. This method won't clean the cells array.
```java
public long sumThenReset() {
Cell[] as = cells; Cell a;
long sum = base;
base = 0L;
if (as != null) {
for (int i = 0; i < as.length; ++i) {
if ((a = as[i]) != null) {
sum += a.value;
a.value = 0L;
}
}
}
return sum;
}
```
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1829872164
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -265,6 +265,10 @@ private[celeborn] class Master(
}).sum()
}
+ masterSource.addGauge(MasterSource.SHUFFLE_FALLBACK_COUNT) { () =>
Review Comment:
thanks for the comments, checking
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "RexXiong (via GitHub)" <gi...@apache.org>.
RexXiong commented on PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#issuecomment-2459832477
> > why not directly use Counter?
>
> Hi @RexXiong, FYI: [#2866 (comment)](https://github.com/apache/celeborn/pull/2866#discussion_r1828996937)
IMO, the speed of fallback is not very significant, because the fallback itself is a somewhat random occurrence.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825492906
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
I am worried that the the LongAdder maintain an array for the added Long number.
And it might be heavy if we always add element into it and does not make compaction.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825494248
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
Can we make aggregation here?
https://github.com/apache/celeborn/blob/41fdb8ade10b97615533438607a2cb1add3a4232/master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java#L500
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "gaoyajun02 (via GitHub)" <gi...@apache.org>.
gaoyajun02 commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r2009851715
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -265,6 +265,10 @@ private[celeborn] class Master(
}).sum()
}
+ masterSource.addGauge(MasterSource.SHUFFLE_FALLBACK_COUNT) { () =>
Review Comment:
"Looking at the final merged code, it's still using addGauge instead of addMeter. Was this change overlooked?"
@turboFei @RexXiong
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -265,6 +265,10 @@ private[celeborn] class Master(
}).sum()
}
+ masterSource.addGauge(MasterSource.SHUFFLE_FALLBACK_COUNT) { () =>
Review Comment:
Looking at the final merged code, it's still using addGauge instead of addMeter. Was this change overlooked?
@turboFei @RexXiong
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#issuecomment-2748238382
> > > why not directly use Counter?
>
> >
>
> > Hi @RexXiong, FYI: [#2866 (comment)](https://github.com/apache/celeborn/pull/2866#discussion_r1828996937)
>
>
>
> IMO, the speed of fallback is not very significant, because the fallback itself is a somewhat random occurrence. cc @FMX
cc @gaoyajun02
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1828996937
##########
master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala:
##########
@@ -265,6 +265,10 @@ private[celeborn] class Master(
}).sum()
}
+ masterSource.addGauge(MasterSource.SHUFFLE_FALLBACK_COUNT) { () =>
Review Comment:
Although the gauge can describe the total count of fallback, I think the meters metrics will be more helpful in showing the status of current clusters and jobs.
The speed of fallback can be more intuitive.
https://metrics.dropwizard.io/4.2.0/manual/core.html#meters
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#issuecomment-2461215755
> Okay, dashboard.json need change too.
Thanks for the reminder
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825495221
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
The master will update the partitionSize periodically.
https://github.com/apache/celeborn/blob/41fdb8ade10b97615533438607a2cb1add3a4232/master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala#L211-L223
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
The master updates the partitionSize periodically.
https://github.com/apache/celeborn/blob/41fdb8ade10b97615533438607a2cb1add3a4232/master/src/main/scala/org/apache/celeborn/service/deploy/master/Master.scala#L211-L223
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825494248
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
Can we make aggregation here?
https://github.com/apache/celeborn/blob/41fdb8ade10b97615533438607a2cb1add3a4232/master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java#495-L500
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
Can we make aggregation here?
https://github.com/apache/celeborn/blob/41fdb8ade10b97615533438607a2cb1add3a4232/master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java#L495-L500
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "FMX (via GitHub)" <gi...@apache.org>.
FMX commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825490563
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
The value of shuffleFallbackCount is the count for the interval of two application hearts, just adding it is OK.
```suggestion
shuffleTotalFallbackCount.add(shuffleFallbackCount);
```
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825646376
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
thanks a lot.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825645723
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
Ok, thanks for the info. I misunderstood the resizing.
There is a probe value for the thread that update the LongAdder, it is fine.
```
/**
* Returns the probe value for the current thread.
* Duplicated from ThreadLocalRandom because of packaging restrictions.
*/
static final int getProbe() {
return UNSAFE.getInt(Thread.currentThread(), PROBE);
}
```
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#issuecomment-2459026692
> why not directly use Counter?
Hi @RexXiong, FYI: https://github.com/apache/celeborn/pull/2866#discussion_r1828996937
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#issuecomment-2460840619
> because the fallback itself is a somewhat random occurrence
Hi @RexXiong , I am fine for this, will just change the metrics type and reserve the code for Meter metrics in case we need it in the future.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825492906
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
I am worried that the the LongAdder maintain an array for the added Long number.
And it might be heavy to the sum of this array if we always add element into it and does not make compaction.
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
I am worried that the the LongAdder maintain an array for the added Long number.
And it might be heavy to the sum of this array if we always add element into it and does not make aggregation.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-1680] Introduce ShuffleFallbackCount metrics [celeborn]
Posted by "turboFei (via GitHub)" <gi...@apache.org>.
turboFei commented on code in PR #2866:
URL: https://github.com/apache/celeborn/pull/2866#discussion_r1825492906
##########
master/src/main/java/org/apache/celeborn/service/deploy/master/clustermeta/AbstractMetaManager.java:
##########
@@ -139,10 +140,12 @@ public void updateBatchUnregisterShuffleMeta(List<String> shuffleKeys) {
}
}
- public void updateAppHeartbeatMeta(String appId, long time, long totalWritten, long fileCount) {
+ public void updateAppHeartbeatMeta(
+ String appId, long time, long totalWritten, long fileCount, long shuffleFallbackCount) {
appHeartbeatTime.put(appId, time);
partitionTotalWritten.add(totalWritten);
partitionTotalFileCount.add(fileCount);
+ shuffleTotalFallbackCount.add(shuffleTotalFallbackCount.sumThenReset() + shuffleFallbackCount);
Review Comment:
I am worried that the the LongAdder maintain an array for the added Long numbers.
And it might be heavy to get the sum of this array if we always add element into it and does not make aggregation.
--
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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org