You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Bill Farner <wf...@apache.org> on 2015/07/11 00:47:33 UTC
Review Request 36407: Introduce DB entity objects and avoid ugly hacks
around mybatis/thrift issues.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36407/
-----------------------------------------------------------
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
Re: Review Request 36407: Introduce DB entity objects and avoid ugly
hacks around mybatis/thrift issues.
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36407/#review91559
-----------------------------------------------------------
Ship it!
Ship It!
- 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
>
>
Re: Review Request 36407: Introduce DB entity objects and avoid ugly
hacks around mybatis/thrift issues.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36407/#review91363
-----------------------------------------------------------
Ship it!
Master (190daed) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
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
>
>
Re: Review Request 36407: Introduce DB entity objects and avoid ugly
hacks around mybatis/thrift issues.
Posted by Zameer Manji <zm...@apache.org>.
> On July 13, 2015, 3: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.
>
> Bill Farner wrote:
> 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.
SGTM.
- 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
>
>
Re: Review Request 36407: Introduce DB entity objects and avoid ugly
hacks around mybatis/thrift issues.
Posted by Zameer Manji <zm...@apache.org>.
> 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
>
>
Re: Review Request 36407: Introduce DB entity objects and avoid ugly
hacks around mybatis/thrift issues.
Posted by Bill Farner <wf...@apache.org>.
> 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
>
>
Re: Review Request 36407: Introduce DB entity objects and avoid ugly
hacks around mybatis/thrift issues.
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
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
>
>