AmandeepSingh285 commented on code in PR #3740:
URL: https://github.com/apache/celeborn/pull/3740#discussion_r3644768563
##########
client/src/main/scala/org/apache/celeborn/client/ApplicationHeartbeater.scala:
##########
@@ -85,10 +99,14 @@ class ApplicationHeartbeater(
tmpApplicationFallbackCounts.asJava,
workerStatusTracker.getNeedCheckedWorkers().toList.asJava,
ZERO_UUID,
- true)
+ true,
+ if (appMetricLabels.isEmpty)
java.util.Collections.emptyMap[String, ClientMetric]()
+ else clientMetrics(),
+ appMetricLabels)
val response = requestHeartbeat(appHeartbeat)
if (response.statusCode == StatusCode.SUCCESS) {
logDebug("Successfully send app heartbeat.")
+ commitClientMetrics()
Review Comment:
@SteNicholas Thanks for pointing this out. I agree this is a valid concern.
With the current protocol, if the master successfully applies a metrics update
but the heartbeat response is lost, the same counter deltas can indeed be
resent and double-counted on the next heartbeat. Addressing this correctly
would require introducing some idempotent protocol which I think would be
fairly substantial change.
To keep the scope of this PR manageable, I'm thinking we can limit this
change to pushing gauge metrics, since gauges represent the latest value and
are not affected by this ambiguity. We can then follow up with a separate
change to add support for counter or other delta metrics together with an
idempotent protocol to handle ambiguous heartbeat failures correctly.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
