Re: Review Request 43150: Expose MyBatis PoolState via stats

[Maxim Khutornenko]

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


Ship it!





src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java (line 138)


Recommend using statsProvider.untracked() here.


- Maxim Khutornenko


On Feb. 3, 2016, 10:11 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43150/
> ---
> 
> (Updated Feb. 3, 2016, 10:11 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1607
> https://issues.apache.org/jira/browse/AURORA-1607
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> To better understand the MyBatis connection pool this patch exposes the pool 
> state via stats.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 7674b8af6d18b39153ecdf02015f0970e035e874 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 
> 3ab95c615b01f3201e3b87089119abf01d71dbb7 
> 
> Diff: https://reviews.apache.org/r/43150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> Checked the metrics in vagrant
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 43150: Expose MyBatis PoolState via stats

[Zameer Manji]

> On Feb. 3, 2016, 2:19 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, line 
> > 138
> > 
> >
> > Recommend using statsProvider.untracked() here.

What's the reason for that?


- Zameer


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


On Feb. 3, 2016, 2:11 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43150/
> ---
> 
> (Updated Feb. 3, 2016, 2:11 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1607
> https://issues.apache.org/jira/browse/AURORA-1607
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> To better understand the MyBatis connection pool this patch exposes the pool 
> state via stats.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 7674b8af6d18b39153ecdf02015f0970e035e874 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 
> 3ab95c615b01f3201e3b87089119abf01d71dbb7 
> 
> Diff: https://reviews.apache.org/r/43150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> Checked the metrics in vagrant
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 43150: Expose MyBatis PoolState via stats

[Zameer Manji]

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

(Updated Feb. 3, 2016, 2:23 p.m.)


Review request for Aurora, John Sirois and Maxim Khutornenko.


Changes
---

John's feedback.


Bugs: AURORA-1607
https://issues.apache.org/jira/browse/AURORA-1607


Repository: aurora


Description
---

To better understand the MyBatis connection pool this patch exposes the pool 
state via stats.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
7674b8af6d18b39153ecdf02015f0970e035e874 
  src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 
3ab95c615b01f3201e3b87089119abf01d71dbb7 

Diff: https://reviews.apache.org/r/43150/diff/


Testing
---

./gradlew build -Pq
Checked the metrics in vagrant


Thanks,

Zameer Manji

Re: Review Request 43150: Expose MyBatis PoolState via stats

[Aurora ReviewBot]

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


Ship it!




Master (2d91e18) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Feb. 3, 2016, 10:11 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43150/
> ---
> 
> (Updated Feb. 3, 2016, 10:11 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1607
> https://issues.apache.org/jira/browse/AURORA-1607
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> To better understand the MyBatis connection pool this patch exposes the pool 
> state via stats.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 7674b8af6d18b39153ecdf02015f0970e035e874 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 
> 3ab95c615b01f3201e3b87089119abf01d71dbb7 
> 
> Diff: https://reviews.apache.org/r/43150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> Checked the metrics in vagrant
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 43150: Expose MyBatis PoolState via stats

[Zameer Manji]

> On Feb. 3, 2016, 2:08 p.m., John Sirois wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, line 
> > 275
> > 
> >
> > Consider a bit more DRY - _almost_ like having local functions:
> > ```java
> > BiConsumer makeGuage = (suffix, 
> > guage) ->
> > statsProvider.makeGauge("db_storage_mybatis_connection_pool_" + 
> > suffix, guage);
> > 
> > makeGuage.accept("requests", poolState::getRequestCount);
> > ...
> > ```
> > 
> > ... or just extract a second helper function.

Second helper function extracted.


- Zameer


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


On Feb. 3, 2016, 2:23 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43150/
> ---
> 
> (Updated Feb. 3, 2016, 2:23 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1607
> https://issues.apache.org/jira/browse/AURORA-1607
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> To better understand the MyBatis connection pool this patch exposes the pool 
> state via stats.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 7674b8af6d18b39153ecdf02015f0970e035e874 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 
> 3ab95c615b01f3201e3b87089119abf01d71dbb7 
> 
> Diff: https://reviews.apache.org/r/43150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> Checked the metrics in vagrant
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 43150: Expose MyBatis PoolState via stats

[Bill Farner]

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


Ship it!




Ship It!

- Bill Farner


On Feb. 3, 2016, 2:23 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43150/
> ---
> 
> (Updated Feb. 3, 2016, 2:23 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1607
> https://issues.apache.org/jira/browse/AURORA-1607
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> To better understand the MyBatis connection pool this patch exposes the pool 
> state via stats.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 7674b8af6d18b39153ecdf02015f0970e035e874 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 
> 3ab95c615b01f3201e3b87089119abf01d71dbb7 
> 
> Diff: https://reviews.apache.org/r/43150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> Checked the metrics in vagrant
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 43150: Expose MyBatis PoolState via stats

[John Sirois]

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


Ship it!




Ship It!

- John Sirois


On Feb. 3, 2016, 3:23 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43150/
> ---
> 
> (Updated Feb. 3, 2016, 3:23 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1607
> https://issues.apache.org/jira/browse/AURORA-1607
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> To better understand the MyBatis connection pool this patch exposes the pool 
> state via stats.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 7674b8af6d18b39153ecdf02015f0970e035e874 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 
> 3ab95c615b01f3201e3b87089119abf01d71dbb7 
> 
> Diff: https://reviews.apache.org/r/43150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> Checked the metrics in vagrant
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 43150: Expose MyBatis PoolState via stats

[John Sirois]

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


Ship it!




Drive by LGTM.


src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java (line 275)


Consider a bit more DRY - _almost_ like having local functions:
```java
BiConsumer makeGuage = (suffix, guage) 
->
statsProvider.makeGauge("db_storage_mybatis_connection_pool_" + suffix, 
guage);

makeGuage.accept("requests", poolState::getRequestCount);
...
```

... or just extract a second helper function.


- John Sirois


On Feb. 3, 2016, 2:59 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43150/
> ---
> 
> (Updated Feb. 3, 2016, 2:59 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1607
> https://issues.apache.org/jira/browse/AURORA-1607
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> To better understand MyBatis connection pool this patch exposes the pool 
> state via stats.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 7674b8af6d18b39153ecdf02015f0970e035e874 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 
> 3ab95c615b01f3201e3b87089119abf01d71dbb7 
> 
> Diff: https://reviews.apache.org/r/43150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> Checked the metrics in vagrant
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 43150: Expose MyBatis PoolState via stats

[John Sirois]

> On Feb. 3, 2016, 3:19 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, line 
> > 138
> > 
> >
> > Recommend using statsProvider.untracked() here.
> 
> Zameer Manji wrote:
> What's the reason for that?
> 
> Maxim Khutornenko wrote:
> I just see no reason not to :) Unless you want to observe them in 
> /graphview...
> 
> Bill Farner wrote:
> Unless your external stats system polls pretty frequently, O(seconds), 
> you might actually miss useful activity in these gauges.
> 
> Maxim Khutornenko wrote:
> This is the part I am not clear, does the time series matter for the 
> stats collection? Does not it only see the immediate value at the time of 
> collection?

I think that's Bill's point.  Since Guages are instantaneous, if the external 
sampling is, say, every 60s, and the interesting pool starvation happens 10s 
but then corrects 10s later at the 20s mark since last collection, you'll not 
see it ... unless you have tracking enabled and collection at - default - 1s.  
Even then, you'll only see it via /graphview

... rusty recollections warning ...


- John


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


On Feb. 3, 2016, 3:23 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43150/
> ---
> 
> (Updated Feb. 3, 2016, 3:23 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1607
> https://issues.apache.org/jira/browse/AURORA-1607
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> To better understand the MyBatis connection pool this patch exposes the pool 
> state via stats.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 7674b8af6d18b39153ecdf02015f0970e035e874 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 
> 3ab95c615b01f3201e3b87089119abf01d71dbb7 
> 
> Diff: https://reviews.apache.org/r/43150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> Checked the metrics in vagrant
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 43150: Expose MyBatis PoolState via stats

[Maxim Khutornenko]

> On Feb. 3, 2016, 10:19 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, line 
> > 138
> > 
> >
> > Recommend using statsProvider.untracked() here.
> 
> Zameer Manji wrote:
> What's the reason for that?
> 
> Maxim Khutornenko wrote:
> I just see no reason not to :) Unless you want to observe them in 
> /graphview...
> 
> Bill Farner wrote:
> Unless your external stats system polls pretty frequently, O(seconds), 
> you might actually miss useful activity in these gauges.
> 
> Maxim Khutornenko wrote:
> This is the part I am not clear, does the time series matter for the 
> stats collection? Does not it only see the immediate value at the time of 
> collection?
> 
> John Sirois wrote:
> I think that's Bill's point.  Since Guages are instantaneous, if the 
> external sampling is, say, every 60s, and the interesting pool starvation 
> happens 10s but then corrects 10s later at the 20s mark since last 
> collection, you'll not see it ... unless you have tracking enabled and 
> collection at - default - 1s.  Even then, you'll only see it via /graphview
> 
> ... rusty recollections warning ...
> 
> Bill Farner wrote:
> John's recollection is correct.

Aha, then we are on the same page. 

I guess the ideal would be collecting max counters instead? That way we would 
not miss anything and catch spikes with rate()?


- Maxim


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


On Feb. 3, 2016, 10:23 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43150/
> ---
> 
> (Updated Feb. 3, 2016, 10:23 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1607
> https://issues.apache.org/jira/browse/AURORA-1607
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> To better understand the MyBatis connection pool this patch exposes the pool 
> state via stats.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 7674b8af6d18b39153ecdf02015f0970e035e874 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 
> 3ab95c615b01f3201e3b87089119abf01d71dbb7 
> 
> Diff: https://reviews.apache.org/r/43150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> Checked the metrics in vagrant
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 43150: Expose MyBatis PoolState via stats

[Maxim Khutornenko]

> On Feb. 3, 2016, 10:19 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java, line 
> > 138
> > 
> >
> > Recommend using statsProvider.untracked() here.
> 
> Zameer Manji wrote:
> What's the reason for that?
> 
> Maxim Khutornenko wrote:
> I just see no reason not to :) Unless you want to observe them in 
> /graphview...
> 
> Bill Farner wrote:
> Unless your external stats system polls pretty frequently, O(seconds), 
> you might actually miss useful activity in these gauges.

This is the part I am not clear, does the time series matter for the stats 
collection? Does not it only see the immediate value at the time of collection?


- Maxim


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


On Feb. 3, 2016, 10:23 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43150/
> ---
> 
> (Updated Feb. 3, 2016, 10:23 p.m.)
> 
> 
> Review request for Aurora, John Sirois and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1607
> https://issues.apache.org/jira/browse/AURORA-1607
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> To better understand the MyBatis connection pool this patch exposes the pool 
> state via stats.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 7674b8af6d18b39153ecdf02015f0970e035e874 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbStorageTest.java 
> 3ab95c615b01f3201e3b87089119abf01d71dbb7 
> 
> Diff: https://reviews.apache.org/r/43150/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> Checked the metrics in vagrant
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>