bkonold commented on a change in pull request #1158: SAMZA-2324: Adding KV
store metrics for rocksdb
URL: https://github.com/apache/samza/pull/1158#discussion_r325434317
##########
File path:
samza-kv/src/main/scala/org/apache/samza/storage/kv/SerializedKeyValueStore.scala
##########
@@ -55,23 +55,27 @@ class SerializedKeyValueStore[K, V](
}
def put(key: K, value: V) {
- metrics.puts.inc
val keyBytes = toBytesOrNull(key, keySerde)
val valBytes = toBytesOrNull(value, msgSerde)
store.put(keyBytes, valBytes)
+ metrics.puts.inc
+
metrics.maxRecordSizeBytes.set(Math.max(metrics.maxRecordSizeBytes.getValue,
valBytes.length))
Review comment:
Ah, good point. From a more general perspective, since metrics in samza are
designed in a "poll for the value" fashion, this already lends itself to lossy
metrics data. Take MetricsSnapshotReporter for example, which will poll the
registered metrics at some cadence and only see the value that is set at the
particular polling interval.
Nothing here should be getting to a bad state like you mention, we'd just be
potentially recording values that aren't _actually_ the true maximum.
The problem may be exaggerated for jobs where message size is highly
variable however. I think in general this problem would be best addressed by
recording gauge metrics as a distribution rather than a single value, but
that's out of scope for this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
