aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 35498: Compute SLA stats for non-prod jobs
Date Mon, 29 Jun 2015 15:37:52 GMT


> On June 25, 2015, 5:59 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java, lines 138-139
> > <https://reviews.apache.org/r/35498/diff/7/?file=991483#file991483line138>
> >
> >     Add getters for these fields and access them below via the getters rather than
direct field access.
> 
> Stephan Erb wrote:
>     I can submit an updated patch tonight. I've somewhat expected that some of you would
point it out :-)
>     
>     I thought about adding these when writing the patch but then decided against them:
Getters for final attributes on an inner class did not seem to offer any meaningful encapsulation
or help during refactoring.
> 
> Kevin Sweeney wrote:
>     In this case the class in question uses getters and direct field access inconsistently,
I'd also be okay with removing all getters on that class and replacing them with direct field
access. I think getters are more readable though (it's a code review red flag when I see a
new direct field access). Other committers feel free to chime in here.
> 
> Stephan Erb wrote:
>     Your consistency argument is very valid. I guess we can leave it as it is.

IMO, using getters in cases like this feels redundant and just creates ground for inconsistent
field/method reference. I'd drop getters as they provide zero value here.


- Maxim


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


On June 26, 2015, 7 p.m., Stephan Erb wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35498/
> -----------------------------------------------------------
> 
> (Updated June 26, 2015, 7 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1350
>     https://issues.apache.org/jira/browse/AURORA-1350
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Compute SLA stats for non-prod jobs
> 
> This is a first iteration closely following the design proposal of Maxim as posted on
the mailinglist. Feedback welcome.
> 
> 
> Diffs
> -----
> 
>   NEWS fd5b70823cf7a695831d34453ab101c7d6a626e8 
>   docs/sla.md 14e9108fda91200bbf56384c96b9cd926689311f 
>   src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java 82f36d5ca15df18bdc8ebbbd868d3394db38e603

>   src/main/java/org/apache/aurora/scheduler/sla/SlaAlgorithm.java ff73ca6265bd0699791da5e5b6ed4aab9156d9e4

>   src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 64e986fb2e0f955dd4a9c7824eac9494728bf24e

>   src/test/java/org/apache/aurora/scheduler/sla/MetricCalculatorTest.java cb98834e925793fc116814371548a30470830164

>   src/test/java/org/apache/aurora/scheduler/sla/SlaModuleTest.java 5ee123a03e3c8670e0c03b05c48a9f4c66f6af9d

> 
> Diff: https://reviews.apache.org/r/35498/diff/
> 
> 
> Testing
> -------
> 
> `./gradlew -Pq build` and a manual verification in Vagrant.
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message