You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@celeborn.apache.org by "xy2953396112 (via GitHub)" <gi...@apache.org> on 2025/08/16 02:47:09 UTC
[PR] [CELEBORN-2105] When Inbox stopped, clean the RpcMetricsTracker [celeborn]
xy2953396112 opened a new pull request, #3419:
URL: https://github.com/apache/celeborn/pull/3419
<!--
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?
When Inbox stopped, clean the RpcMetricsTracker.
--
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-2105] When Inbox stopped, clean the RpcMetricsTracker [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 commented on PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#issuecomment-3275069059
@SteNicholas Please help review this pr, thanks~
--
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-2105] When Inbox stopped, clean the RpcMetricsTracker [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 commented on code in PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#discussion_r2477980690
##########
common/src/main/scala/org/apache/celeborn/common/rpc/RpcMetricsTracker.scala:
##########
@@ -67,6 +67,15 @@ private[celeborn] class RpcMetricsTracker(
}
}
+ def clear(): Unit = {
Review Comment:
Change to `close`.
--
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-2105] RpcMetricsTracker should clean up metrics for stopping Inbox [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 commented on code in PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#discussion_r2494209530
##########
common/src/main/scala/org/apache/celeborn/common/rpc/netty/Inbox.scala:
##########
@@ -285,6 +285,7 @@ private[celeborn] class Inbox(
// Note: The concurrent events in messages will be processed one by one.
}
} finally {
+ metrics.close()
Review Comment:
Moved `metrics.dump()` before `addMessage(OnStop)`.
--
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-2105] RpcMetricsTracker should clean up metrics for stopping Inbox [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas closed pull request #3419: [CELEBORN-2105] RpcMetricsTracker should clean up metrics for stopping Inbox
URL: https://github.com/apache/celeborn/pull/3419
--
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-2105] When Inbox stopped, clean the RpcMetricsTracker [celeborn]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#issuecomment-3355311873
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2105] RpcMetricsTracker should clean up metrics for stopping Inbox [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#discussion_r2489361172
##########
common/src/main/scala/org/apache/celeborn/common/rpc/netty/Inbox.scala:
##########
@@ -282,6 +282,7 @@ private[celeborn] class Inbox(
stopped = true
addMessage(OnStop)
metrics.dump()
+ metrics.close()
Review Comment:
Why not invoke `close` in finally blocker?
--
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-2105] RpcMetricsTracker should clean up metrics for stopping Inbox [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#issuecomment-3500151445
Thanks. Merged to main(v0.7.0) and branch-0.6(v0.6.2).
--
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-2105] When Inbox stopped, clean the RpcMetricsTracker [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#discussion_r2489354910
##########
common/src/main/scala/org/apache/celeborn/common/metrics/source/AbstractSource.scala:
##########
@@ -183,6 +183,15 @@ abstract class AbstractSource(conf: CelebornConf, role: String)
})
}
+ def removeTimer(name: String): Unit = removeTimer(name, Map.empty[String, String])
+
+ def removeTimer(name: String, labels: Map[String, String]): Unit = {
+ val metricNameWithLabel = metricNameWithCustomizedLabels(name, labels)
+ if (metricRegistry.getTimers.containsKey(metricNameWithLabel)) {
Review Comment:
Why not remove `metricNameWithLabel` from `metricRegistry`? Then, it's unnecessary to remove `metricNameWithLabel` from `namedTimers` after checking.
##########
common/src/main/scala/org/apache/celeborn/common/metrics/source/AbstractSource.scala:
##########
@@ -183,6 +183,15 @@ abstract class AbstractSource(conf: CelebornConf, role: String)
})
}
+ def removeTimer(name: String): Unit = removeTimer(name, Map.empty[String, String])
+
+ def removeTimer(name: String, labels: Map[String, String]): Unit = {
+ val metricNameWithLabel = metricNameWithCustomizedLabels(name, labels)
+ if (metricRegistry.getTimers.containsKey(metricNameWithLabel)) {
Review Comment:
Why not remove `metricNameWithLabel` from `metricRegistry`? Then, it's unnecessary to remove `metricNameWithLabel` from `namedTimers` after 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-2105] When Inbox stopped, clean the RpcMetricsTracker [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#discussion_r2477845164
##########
common/src/main/scala/org/apache/celeborn/common/rpc/RpcMetricsTracker.scala:
##########
@@ -67,6 +67,15 @@ private[celeborn] class RpcMetricsTracker(
}
}
+ def clear(): Unit = {
+ histogramMap.clear()
+ rpcSource.removeGauge(RpcSource.QUEUE_LENGTH)
+ rpcSource.removeTimer(RpcSource.QUEUE_TIME)
+ rpcSource.removeTimer(RpcSource.PROCESS_TIME)
+ logInfo(s"clear rpc metrics tracker for $name: ${RpcSource.QUEUE_LENGTH}, " +
Review Comment:
It's unnecessary to log 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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2105] When Inbox stopped, clean the RpcMetricsTracker [celeborn]
Posted by "xy2953396112 (via GitHub)" <gi...@apache.org>.
xy2953396112 commented on code in PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#discussion_r2477979812
##########
common/src/main/scala/org/apache/celeborn/common/rpc/RpcMetricsTracker.scala:
##########
@@ -67,6 +67,15 @@ private[celeborn] class RpcMetricsTracker(
}
}
+ def clear(): Unit = {
+ histogramMap.clear()
+ rpcSource.removeGauge(RpcSource.QUEUE_LENGTH)
+ rpcSource.removeTimer(RpcSource.QUEUE_TIME)
+ rpcSource.removeTimer(RpcSource.PROCESS_TIME)
+ logInfo(s"clear rpc metrics tracker for $name: ${RpcSource.QUEUE_LENGTH}, " +
Review Comment:
ok, I have removed it.
--
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-2105] When Inbox stopped, clean the RpcMetricsTracker [celeborn]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#issuecomment-3263589202
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 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@celeborn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CELEBORN-2105] When Inbox stopped, clean the RpcMetricsTracker [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#discussion_r2489361172
##########
common/src/main/scala/org/apache/celeborn/common/rpc/netty/Inbox.scala:
##########
@@ -282,6 +282,7 @@ private[celeborn] class Inbox(
stopped = true
addMessage(OnStop)
metrics.dump()
+ metrics.close()
Review Comment:
Why not invoke `close` in finnaly blocker?
##########
common/src/main/scala/org/apache/celeborn/common/metrics/source/AbstractSource.scala:
##########
@@ -183,6 +183,15 @@ abstract class AbstractSource(conf: CelebornConf, role: String)
})
}
+ def removeTimer(name: String): Unit = removeTimer(name, Map.empty[String, String])
+
+ def removeTimer(name: String, labels: Map[String, String]): Unit = {
+ val metricNameWithLabel = metricNameWithCustomizedLabels(name, labels)
+ if (metricRegistry.getTimers.containsKey(metricNameWithLabel)) {
Review Comment:
Why not remove metricNameWithLabel from metricRegistry? Then, it's unnecessary to remove metricNameWithLabel from namedTimers after 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-2105] RpcMetricsTracker should clean up metrics for stopping Inbox [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#discussion_r2493753190
##########
common/src/main/scala/org/apache/celeborn/common/rpc/netty/Inbox.scala:
##########
@@ -285,6 +285,7 @@ private[celeborn] class Inbox(
// Note: The concurrent events in messages will be processed one by one.
}
} finally {
+ metrics.close()
Review Comment:
fDoes `metric#dump` need to move into finally blocker? Because `addMessage` may throw exception, which causes that metric would close before dumping.
##########
common/src/main/scala/org/apache/celeborn/common/rpc/netty/Inbox.scala:
##########
@@ -285,6 +285,7 @@ private[celeborn] class Inbox(
// Note: The concurrent events in messages will be processed one by one.
}
} finally {
+ metrics.close()
Review Comment:
Does `metric#dump` need to move into finally blocker? Because `addMessage` may throw exception, which causes that metric would close before dumping.
--
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-2105] When Inbox stopped, clean the RpcMetricsTracker [celeborn]
Posted by "SteNicholas (via GitHub)" <gi...@apache.org>.
SteNicholas commented on code in PR #3419:
URL: https://github.com/apache/celeborn/pull/3419#discussion_r2477835440
##########
common/src/main/scala/org/apache/celeborn/common/rpc/RpcMetricsTracker.scala:
##########
@@ -67,6 +67,15 @@ private[celeborn] class RpcMetricsTracker(
}
}
+ def clear(): Unit = {
Review Comment:
It's better to rename to `stop` or `close`.
--
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