Re: Review Request 50619: SAMZA-963: add KV storage engine timers to help identify the issues on kv stores and also add unit test

2016-08-08 Thread Yi Pan (Data Infrastructure) 

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


Ship it!




Ship It!

- Yi Pan (Data Infrastructure)


On Aug. 5, 2016, 10:27 p.m., Fred Ji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50619/
> ---
> 
> (Updated Aug. 5, 2016, 10:27 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/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

2016-08-05 Thread Fred Ji 


> 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
> > 
> >
> > 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
> 
>

Re: Review Request 50619: SAMZA-963: add KV storage engine timers to help identify the issues on kv stores and also add unit test

2016-08-05 Thread Yi Pan (Data Infrastructure) 

---
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)


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

2016-07-29 Thread Jake Maes 

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


Ship it!




LGTM

- Jake Maes


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
> 
>

Review Request 50619: SAMZA-963: add KV storage engine timers to help identify the issues on kv stores and also add unit test

2016-07-29 Thread Fred Ji 

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

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