> On July 13, 2015, 3: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? > > Bill Farner wrote: > 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?
I think avoiding the copy is a good reason to not have consistency across these types. So long as this convention can be enforced going forward I think we will be fine. - Zameer ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36407/#review91538 ----------------------------------------------------------- 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 > >
