cameronlee314 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_r327339170
##########
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:
It is this specific read-then-write pattern which is not threadsafe; `Gauge`
itself is threadsafe. It can be argued that it is not `Gauge`'s responsibility
to be able to handle a read-then-write pattern, so I don't think this issue
should be called out there. `Gauge` isn't usually used to store values based on
a previous value of the `Gauge`. I think if the user of `Gauge` wants to do
"max", then the user needs to track the max separately (or use `Histogram`
instead to track distribution, as you mentioned).
----------------------------------------------------------------
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
