aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 36407: Introduce DB entity objects and avoid ugly hacks around mybatis/thrift issues.
Date Mon, 13 Jul 2015 22:32:44 GMT


> On July 13, 2015, 10:04 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java,
line 40
> > <https://reviews.apache.org/r/36407/diff/1/?file=1008765#file1008765line40>
> >
> >     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?

The convention i made up for this was to provide `toThrift()` for entites that are always
contained in others, to avoid a mutable -> immutable -> mutable -> immutable dance
(and copy) as the object tree is created.  Note that these methods are always package-private.
 `toImmutable()` is added for top-level objects, and is the only public method.

That seem reasonable?


> On July 13, 2015, 10:04 p.m., Zameer Manji wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/views/Pairs.java, line 14
> > <https://reviews.apache.org/r/36407/diff/1/?file=1008771#file1008771line14>
> >
> >     Why not move this class to org.apache.aurora.util? Nothing about this utility
class is specific to db views.

I prefer to keep code closest to where it's used.  I would have even made the class package
private, but it is used in two proximal packages.


- Bill


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


On July 10, 2015, 10: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, 10: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
> 
>


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