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/06/26 19:56:40 UTC
Review Request 35933: SAMZA-449 Expose RocksDB statistic
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35933/
-----------------------------------------------------------
Review request for samza.
Repository: samza
Description
-------
RocksDB statistic
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/RocksDbStatistic.scala PRE-CREATION
samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala a428a16bc1e9ab4980a6f17db4fd810057d31136
Diff: https://reviews.apache.org/r/35933/diff/
Testing
-------
Thanks,
Gustavo Anatoly F. V. Solís
Re: Review Request 35933: SAMZA-449 Expose RocksDB statistic
Posted by "Gustavo Anatoly F. V. Solís" <gu...@gmail.com>.
> On Junho 30, 2015, 3:25 p.m., Yan Fang wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatistic.scala, line 25
> > <https://reviews.apache.org/r/35933/diff/1/?file=993429#file993429line25>
> >
> > It's better to use the name RocksDbStatisticMetrics to indicate it is a metrics.
I going to rename the class. Thanks.
> On Junho 30, 2015, 3:25 p.m., Yan Fang wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala, line 150
> > <https://reviews.apache.org/r/35933/diff/1/?file=993428#file993428line150>
> >
> > it should accept the "metrics" (from line 143) as the third parameter.
The third parameter (metrics) will be added.
> On Junho 30, 2015, 3:25 p.m., Yan Fang wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala, line 243
> > <https://reviews.apache.org/r/35933/diff/1/?file=993428#file993428line243>
> >
> > add some doc here, saying something like, calling this method will expose RocksDB statistic to a metric, etc.
The doc will be written
> On Junho 30, 2015, 3:25 p.m., Yan Fang wrote:
> > samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatistic.scala, line 40
> > <https://reviews.apache.org/r/35933/diff/1/?file=993429#file993429line40>
> >
> > can we use the lower case for the all names of metrics? It's better to keep the name convension consistent in the Samza.
ok, this is will be changed
- Gustavo Anatoly
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35933/#review89902
-----------------------------------------------------------
On Junho 26, 2015, 5:56 p.m., Gustavo Anatoly F. V. Solís wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35933/
> -----------------------------------------------------------
>
> (Updated Junho 26, 2015, 5:56 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB statistic
>
>
> 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/RocksDbStatistic.scala PRE-CREATION
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala a428a16bc1e9ab4980a6f17db4fd810057d31136
>
> Diff: https://reviews.apache.org/r/35933/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gustavo Anatoly F. V. Solís
>
>
Re: Review Request 35933: SAMZA-449 Expose RocksDB statistic
Posted by Yan Fang <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35933/#review89902
-----------------------------------------------------------
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala (line 150)
<https://reviews.apache.org/r/35933/#comment142781>
it should accept the "metrics" (from line 143) as the third parameter.
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala (line 243)
<https://reviews.apache.org/r/35933/#comment142780>
add some doc here, saying something like, calling this method will expose RocksDB statistic to a metric, etc.
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatistic.scala (line 25)
<https://reviews.apache.org/r/35933/#comment142776>
It's better to use the name RocksDbStatisticMetrics to indicate it is a metrics.
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatistic.scala (line 40)
<https://reviews.apache.org/r/35933/#comment142777>
can we use the lower case for the all names of metrics? It's better to keep the name convension consistent in the Samza.
- Yan Fang
On June 26, 2015, 5:56 p.m., Gustavo Anatoly F. V. Solís wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35933/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 5:56 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB statistic
>
>
> 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/RocksDbStatistic.scala PRE-CREATION
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala a428a16bc1e9ab4980a6f17db4fd810057d31136
>
> Diff: https://reviews.apache.org/r/35933/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gustavo Anatoly F. V. Solís
>
>
Re: Review Request 35933: SAMZA-449 Expose RocksDB statistic
Posted by Yan Fang <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35933/#review90373
-----------------------------------------------------------
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala (line 133)
<https://reviews.apache.org/r/35933/#comment143410>
we can remove the "KeyValueStoreMetrics" here, right? change to "new RocksDbStatisticMetrics(storeName, options, metrics)"
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala (line 245)
<https://reviews.apache.org/r/35933/#comment143411>
the "[[]]" is not very common. And I do not see anywere in Samza we are using. It's better to remove them, or use {@link} if you want to link to the class.
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatisticMetrics.scala (line 213)
<https://reviews.apache.org/r/35933/#comment143409>
is any code is using those methods (line 213-219)? If not, it's safe to remove them. Also, users can directly call the variable name to get the value, if they want.
- Yan Fang
On July 3, 2015, 11: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/35933/
> -----------------------------------------------------------
>
> (Updated July 3, 2015, 11:05 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDb expose statistic
>
>
> 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/35933/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gustavo Anatoly F. V. Solís
>
>
Re: Review Request 35933: SAMZA-449 Expose RocksDB statistic
Posted by "Gustavo Anatoly F. V. Solís" <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35933/
-----------------------------------------------------------
(Updated Julho 4, 2015, 12:13 a.m.)
Review request for samza.
Repository: samza
Description
-------
RocksDb expose statistic
Diffs (updated)
-----
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/35933/diff/
Testing
-------
Thanks,
Gustavo Anatoly F. V. Solís
Re: Review Request 35933: SAMZA-449 Expose RocksDB statistic
Posted by "Gustavo Anatoly F. V. Solís" <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35933/
-----------------------------------------------------------
(Updated Julho 3, 2015, 11:05 p.m.)
Review request for samza.
Repository: samza
Description (updated)
-------
RocksDb expose statistic
Diffs (updated)
-----
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/35933/diff/
Testing
-------
Thanks,
Gustavo Anatoly F. V. Solís
Re: Review Request 35933: SAMZA-449 Expose RocksDB statistic
Posted by Yan Fang <ya...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35933/#review89913
-----------------------------------------------------------
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbKeyValueStore.scala (line 237)
<https://reviews.apache.org/r/35933/#comment142793>
Besides we let users decide when to call the getStatistic() to update the metrics, we also can call updateRocksDbStatistic in the flush(). This means, everytime we flush the rocksdb, we will update the rocksDbstatistic.
samza-kv-rocksdb/src/main/scala/org/apache/samza/storage/kv/RocksDbStatistic.scala (line 25)
<https://reviews.apache.org/r/35933/#comment142791>
After second thought, I think current approach has a drawback that, everytime users call getStatistic, we will generate a new Counter/Gauge. That is not ideal.
A better approach is, in this metrics class, to create Counter/Gauge, and wrap them together in a method, probabaly called updateRocksDbStatistic(). We will just call this updateRocksDbStatistic in RocksDbKeyValueStore.scala to "update" the metrics, instead of creating new metrics.
- Yan Fang
On June 26, 2015, 5:56 p.m., Gustavo Anatoly F. V. Solís wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35933/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 5:56 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> RocksDB statistic
>
>
> 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/RocksDbStatistic.scala PRE-CREATION
> samza-kv-rocksdb/src/test/scala/org/apache/samza/storage/kv/TestRocksDbKeyValueStore.scala a428a16bc1e9ab4980a6f17db4fd810057d31136
>
> Diff: https://reviews.apache.org/r/35933/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Gustavo Anatoly F. V. Solís
>
>