Re: Review Request 39422: SAMZA-449 Expose RocksDB statistic

[Navina Ramesh]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39422/#review103081
---



samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatisticMetrics.scala
 (line 25)


storeName is not used anywhere in RocksDbStatisticMetrics. I think the 
store name is prefixed to all metrics defined using KeyValueStoreMetrics.



samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatisticMetrics.scala
 (line 97)


Please add documentation for metrics as a part of the code or in the 
configuration.md page. If it is redundant documentation, then providing a link 
to the RocksDB page with the necessary documentation will be sufficient.



samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatisticMetrics.scala
 (line 210)


Can we filter this list of metrics to what is relevant in Samza? For 
example, we don't use (at least not yet) the multiget rocksdb api in Samza and 
we have intentionally turned off WAL in RocksDB as it is redundant.  

The reason for my concern is that often, the metrics reporting system is 
sensitive to the number of metrics a job emits. So, it might be better to keep 
this list to only what is necessary.

Seems like most of these metrics are only useful for someone trying to 
benchmark the application's local state performance. Can we make turning on 
these metrics optional? We can perhaps keep the basics such as number of keys 
read/written as a default and the rest, optional. What do you think?


- Navina Ramesh


On Oct. 18, 2015, 6:05 p.m., Gustavo Anatoly F. V. Solís wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39422/
> ---
> 
> (Updated Oct. 18, 2015, 6:05 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> RocksDb expose statistic - Increased numberOfOperationsAdded
> 
> 
> Diffs
> -
> 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
>  a423f7bd6c43461e051b5fd1f880dd01db785991 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatisticMetrics.scala
>  PRE-CREATION 
>   
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala
>  a428a16bc1e9ab4980a6f17db4fd810057d31136 
> 
> Diff: https://reviews.apache.org/r/39422/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gustavo Anatoly F. V. Solís
> 
>

Re: Review Request 39422: SAMZA-449 Expose RocksDB statistic

[Gustavo Anatoly F . V . Solís]

> On Out. 19, 2015, 12:04 a.m., Navina Ramesh wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatisticMetrics.scala,
> >  line 25
> > 
> >
> > storeName is not used anywhere in RocksDbStatisticMetrics. I think the 
> > store name is prefixed to all metrics defined using KeyValueStoreMetrics.

You're right the storeName should be prefixed, let me take care of it.

Thanks.


> On Out. 19, 2015, 12:04 a.m., Navina Ramesh wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatisticMetrics.scala,
> >  line 97
> > 
> >
> > Please add documentation for metrics as a part of the code or in the 
> > configuration.md page. If it is redundant documentation, then providing a 
> > link to the RocksDB page with the necessary documentation will be 
> > sufficient.

Let me check out the RocksDB documentation and if it's viable to link it. 

Thanks.


> On Out. 19, 2015, 12:04 a.m., Navina Ramesh wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatisticMetrics.scala,
> >  line 210
> > 
> >
> > Can we filter this list of metrics to what is relevant in Samza? For 
> > example, we don't use (at least not yet) the multiget rocksdb api in Samza 
> > and we have intentionally turned off WAL in RocksDB as it is redundant.  
> > 
> > The reason for my concern is that often, the metrics reporting system 
> > is sensitive to the number of metrics a job emits. So, it might be better 
> > to keep this list to only what is necessary.
> > 
> > Seems like most of these metrics are only useful for someone trying to 
> > benchmark the application's local state performance. Can we make turning on 
> > these metrics optional? We can perhaps keep the basics such as number of 
> > keys read/written as a default and the rest, optional. What do you think?

"So, it might be better to keep this list to only what is necessary." 

I agree, to keep the only essencial.

"Seems like most of these metrics are only useful for someone trying to 
benchmark the application's local state performance. Can we make turning on 
these metrics optional?"

Nice, it's always a good idea to give the flexibility to the user choose what 
want to use.

"We can perhaps keep the basics such as number of keys read/written as a 
default and the rest, optional."

I agree to keep default R/W option.

Thanks.


- Gustavo Anatoly


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39422/#review103081
---


On Out. 18, 2015, 6:05 p.m., Gustavo Anatoly F. V. Solís wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39422/
> ---
> 
> (Updated Out. 18, 2015, 6:05 p.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> ---
> 
> RocksDb expose statistic - Increased numberOfOperationsAdded
> 
> 
> Diffs
> -
> 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala
>  a423f7bd6c43461e051b5fd1f880dd01db785991 
>   
> samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatisticMetrics.scala
>  PRE-CREATION 
>   
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala
>  a428a16bc1e9ab4980a6f17db4fd810057d31136 
> 
> Diff: https://reviews.apache.org/r/39422/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gustavo Anatoly F. V. Solís
> 
>