> On Jan. 21, 2014, 3:29 p.m., Suman Karumuri wrote:
> > src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java, line 
> > 456
> > <https://reviews.apache.org/r/17131/diff/1/?file=432900#file432900line456>
> >
> >     The name CronJob sounds too generic. Consider changing it to 
> > SanitizedCronJob or a similar name that captures intent.

Done.  However, i generally favor brevity/readability when scope is as limited 
as it is here.  The real intention was to put a type in the way to ensure that 
sanitization is performed.


> On Jan. 21, 2014, 3:29 p.m., Suman Karumuri wrote:
> > src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java, line 
> > 461
> > <https://reviews.apache.org/r/17131/diff/1/?file=432900#file432900line461>
> >
> >     inline this variable?

Thanks, done.


> On Jan. 21, 2014, 3:29 p.m., Suman Karumuri wrote:
> > src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java, 
> > line 192
> > <https://reviews.apache.org/r/17131/diff/1/?file=432901#file432901line192>
> >
> >     Would it be better if these comments are changed to JavaDoc comments?

I'm not a fan of that idea since javadoc is really intended to be run through 
the javadoc tool for formal API documentation, which this is not.  It would 
also force me to document the exception thrown, which wouldn't offer any value.


- Bill


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


On Jan. 20, 2014, 9:01 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17131/
> -----------------------------------------------------------
> 
> (Updated Jan. 20, 2014, 9:01 p.m.)
> 
> 
> Review request for Aurora, Suman Karumuri and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-62
>     https://issues.apache.org/jira/browse/AURORA-62
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This raises instruction test coverage from 76% to 95%, and branch coverage 
> from 75% to 96%.
> 
> There are only two things not currently covered:
> - Handling when catching InterruptedException (this only logs)
> - Handling unknown CollisionPolicy (only logs)
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/state/CronJobManager.java 
> 5a56a701a6a355f9de3f05fbb95013b96b06b171 
>   src/test/java/org/apache/aurora/scheduler/state/CronJobManagerTest.java 
> e9886cdb279cc42a961d6c964e2cfae3c4c13f61 
> 
> Diff: https://reviews.apache.org/r/17131/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to