You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com> on 2016/08/05 18:31:00 UTC
Re: Review Request 50619: SAMZA-963: add KV storage engine timers to
help identify the issues on kv stores and also add unit test
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50619/#review144968
-----------------------------------------------------------
Ship it!
LGTM. Just one minor comment. Thanks!
samza-kv/src/main/scala/org/apache/samza/storage/kv/BaseKeyValueStorageEngineFactory.scala (line 135)
<https://reviews.apache.org/r/50619/#comment211122>
Why do we need to add the default function, if you already defined the default in KeyValueStorageEngine.scala?
- Yi Pan (Data Infrastructure)
On July 29, 2016, 10:24 p.m., Fred Ji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50619/
> -----------------------------------------------------------
>
> (Updated July 29, 2016, 10:24 p.m.)
>
>
> Review request for samza, Chris Pettitt, Jake Maes, and Yi Pan (Data Infrastructure).
>
>
> Bugs: SAMZA-963
> https://issues.apache.org/jira/browse/SAMZA-963
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-963: add KV storage engine timers to help identify the issues on kv stores and also add unit test
>
>
> Diffs
> -----
>
> samza-kv/src/main/scala/org/apache/samza/storage/kv/BaseKeyValueStorageEngineFactory.scala c975893a42689732c39c39600fecacee843bf9d6
> samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngine.scala a3ffc421020b7a84c40b2101f2e37db8a20690cb
> samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngineMetrics.scala 233fba91caf041bfb78189efef00ce8fc56f9f15
> samza-kv/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStorageEngine.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50619/diff/
>
>
> Testing
> -------
>
> 1. unit test is successful on a newly created test file for KeyValueStorageEngine: ./gradlew clean :samza-kv_2.10:test -Dtest.single=TestKeyValueStorageEngine
> 2. build and all unit tests are successful: ./gradlew clean build
> 3. ./gradlew checkstyleMain checkstyleTest passed
> 4. manually tested on local machine for a stateful sample job depending on KVStore, and from jconsole, the corresponding metrics were seen in mbeans (see attached file) and the metrics were updated as expected.
>
>
> File Attachments
> ----------------
>
> snapshot of KeyValueStorageEngineMetrics bean from jconsole on local test
> https://reviews.apache.org/media/uploaded/files/2016/07/29/ce4b0456-73f9-44d5-af7d-0c45bf91bcaa__KeyValueStorageEngineMetricsFromJconsole.png
>
>
> Thanks,
>
> Fred Ji
>
>
Re: Review Request 50619: SAMZA-963: add KV storage engine timers to
help identify the issues on kv stores and also add unit test
Posted by Fred Ji <fj...@linkedin.com>.
> On Aug. 5, 2016, 6:31 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-kv/src/main/scala/org/apache/samza/storage/kv/BaseKeyValueStorageEngineFactory.scala, line 135
> > <https://reviews.apache.org/r/50619/diff/1/?file=1458229#file1458229line135>
> >
> > Why do we need to add the default function, if you already defined the default in KeyValueStorageEngine.scala?
Thanks a lot. My initial consideration was to minimize potential errors if somebody would like to add new paramters for this constructor since there are a lot already. I also saw some similar uses in the existing samza code base as well.
I am OK on either one. I can follow whatever code standard our team has.
- Fred
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50619/#review144968
-----------------------------------------------------------
On July 29, 2016, 10:24 p.m., Fred Ji wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50619/
> -----------------------------------------------------------
>
> (Updated July 29, 2016, 10:24 p.m.)
>
>
> Review request for samza, Chris Pettitt, Jake Maes, and Yi Pan (Data Infrastructure).
>
>
> Bugs: SAMZA-963
> https://issues.apache.org/jira/browse/SAMZA-963
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-963: add KV storage engine timers to help identify the issues on kv stores and also add unit test
>
>
> Diffs
> -----
>
> samza-kv/src/main/scala/org/apache/samza/storage/kv/BaseKeyValueStorageEngineFactory.scala c975893a42689732c39c39600fecacee843bf9d6
> samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngine.scala a3ffc421020b7a84c40b2101f2e37db8a20690cb
> samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngineMetrics.scala 233fba91caf041bfb78189efef00ce8fc56f9f15
> samza-kv/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStorageEngine.scala PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50619/diff/
>
>
> Testing
> -------
>
> 1. unit test is successful on a newly created test file for KeyValueStorageEngine: ./gradlew clean :samza-kv_2.10:test -Dtest.single=TestKeyValueStorageEngine
> 2. build and all unit tests are successful: ./gradlew clean build
> 3. ./gradlew checkstyleMain checkstyleTest passed
> 4. manually tested on local machine for a stateful sample job depending on KVStore, and from jconsole, the corresponding metrics were seen in mbeans (see attached file) and the metrics were updated as expected.
>
>
> File Attachments
> ----------------
>
> snapshot of KeyValueStorageEngineMetrics bean from jconsole on local test
> https://reviews.apache.org/media/uploaded/files/2016/07/29/ce4b0456-73f9-44d5-af7d-0c45bf91bcaa__KeyValueStorageEngineMetricsFromJconsole.png
>
>
> Thanks,
>
> Fred Ji
>
>