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