You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by "Gustavo Anatoly F. V. Solís" <gu...@gmail.com> on 2015/10/18 20:05:12 UTC
Review Request 39422: SAMZA-449 Expose RocksDB statistic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39422/
-----------------------------------------------------------
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
Posted by "Gustavo Anatoly F. V. Solís" <gu...@gmail.com>.
> 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
> > <https://reviews.apache.org/r/39422/diff/1/?file=1100579#file1100579line25>
> >
> > 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
> > <https://reviews.apache.org/r/39422/diff/1/?file=1100579#file1100579line97>
> >
> > 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
> > <https://reviews.apache.org/r/39422/diff/1/?file=1100579#file1100579line210>
> >
> > 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
>
>
Re: Review Request 39422: SAMZA-449 Expose RocksDB statistic
Posted by Navina Ramesh <nr...@linkedin.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/39422/#comment160937>
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)
<https://reviews.apache.org/r/39422/#comment160936>
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)
<https://reviews.apache.org/r/39422/#comment160935>
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
>
>