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


Overall LGTM, just two issues to discuss.


src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java
 (line 34)
<https://reviews.apache.org/r/36407/#comment144962>

    Some db view classes provide methods to convert to the immutable type and 
others provide methods to the mutable type. What's the rationale for the 
difference? Why can't they all return toImmutable and let the caller convert as 
needed?



src/main/java/org/apache/aurora/scheduler/storage/db/views/Pairs.java (line 14)
<https://reviews.apache.org/r/36407/#comment144964>

    Why not move this class to org.apache.aurora.util? Nothing about this 
utility class is specific to db views.


- Zameer Manji


On July 10, 2015, 3:47 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36407/
> -----------------------------------------------------------
> 
> (Updated July 10, 2015, 3:47 p.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> I originally went down this path in an attempt to address AURORA-1395 [1], on 
> a hunch that it was caused by mutating objects coming out of the cache (which 
> are expected to be immutable).  While this did not fix AURORA-1395, it 
> significantly cleaned up DbTaskStore, TaskConfigManager, and DbCronJobStore.
> 
> There are some changes in this patch that are detected as moves, thanks to 
> the large license header and git's move tracking.
> 
> [1] https://issues.apache.org/jira/browse/AURORA-1395
> 
> 
> Diffs
> -----
> 
>   config/findbugs/excludeFilter.xml e1c2503e0c6126c098063e0860f36cebbfe567ee 
>   config/pmd/custom.xml 8ad62280f69db0fe185be0005225cf2d65d62383 
>   src/main/java/org/apache/aurora/GuavaUtils.java 
> 9903299f37179fb4081a67859d4a260527123656 
>   src/main/java/org/apache/aurora/scheduler/storage/db/CronJobMapper.java 
> a5c6e997f7f77c3ed1c760d1ab3023249235bcf5 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbCronJobStore.java 
> 664255fbe6a393af83fcdd8d84eb1987eaff103c 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> 0f56411d318e6b05d1ea5ae18dd2dc8111264d2c 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> 2ec6b2d24b16730841d9d4e30fca49d745546862 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> 19f67ad6fd6578f485889398cc45803ca85de58b 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskMapper.java 
> 7d499af58bdc34167c6b7d4970a2938c996e7ce3 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java 
> 07a991d34811256d5ef5504c432fc2b0973ca53f 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java
>  4990af7ce409eb5de20af1a9535bb30abe39ed45 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/AssignedPort.java 
> 0c6442c90289741aa4c59925a6d7a9e4050bf657 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/CronJobWrapper.java
>  5202db5c20dc172586878169440df0543c22daca 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbJobConfiguration.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbScheduledTask.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/DbTaskConfig.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/Pairs.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/ScheduledTaskWrapper.java
>  3e0a2f72389867e61d33cbf4c003f4ec09703b82 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskConfigRow.java 
> 0160ae3049db555cae9e3d3705c1f0daaeef53d4 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskLink.java 
> 52b09a905006bed2a9f01b8beb72772273b5630f 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/CronJobMapper.xml 
> 3826d9f275eac6ccadc03df6846a035326482d6e 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  33047281a6fadd13b51ef4041f809c923aa50cf4 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
> 0b9e3d7d39aa83bbc7e05432638e1b0d50146cda 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/RowGarbageCollectorTest.java
>  00bf3b7df8ae20e0b1c229b1d3c6afcfeb25b403 
> 
> Diff: https://reviews.apache.org/r/36407/diff/
> 
> 
> Testing
> -------
> 
> Note that tests are minimally affected by this change, as it is all hidden 
> behind Store interfaces.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to