aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Stephan Erb" <step...@dev.static-void.de>
Subject Re: Review Request 35498: Compute SLA stats for non-prod jobs
Date Sun, 05 Jul 2015 19:36:06 GMT


> On June 25, 2015, 7: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.
> 
> Maxim Khutornenko wrote:
>     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.

I've changed the code back to not using any getters where possible.

Anything still missing, or can we get this merged?


- Stephan


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


On July 1, 2015, 7:09 p.m., Stephan Erb wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35498/
> -----------------------------------------------------------
> 
> (Updated July 1, 2015, 7:09 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
> 
> 
> 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