> On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 265
> > <https://reviews.apache.org/r/33612/diff/2/?file=950455#file950455line265>
> >
> >     This seems unrelated to the description in this diff.

It is related, as we don't have a `getContainer()` method to use in the diff 
without this change (or a fix to the linked bug).


> On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 131
> > <https://reviews.apache.org/r/33612/diff/2/?file=950460#file950460line131>
> >
> >     Presumably eager cleanup is expensive performance-wise. If that's the 
> > case would you mind calling that out explicitly in the comment?

Are you assuming i added a close delay because of perf?  If so, that's not the 
reason - please see the previous comment rounds with Maxim.


> On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 
> > 154
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line154>
> >
> >     Is there an explicit design choice driving an DELETE+INSERT combination 
> > rather than an UPDATE+INSERT? If so can you call it out? If not can you 
> > drop a TODO to evaluate the tradeoffs of the switch?

This was discussed in previous rounds with Maxim, and a TODO exists on line 
148.  Please let me know if there's something more you're after.


> On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, 
> > lines 259-262
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line259>
> >
> >     Same question as above here - why DELETE+INSERT instead of UPDATE?

^^


> On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java,
> >  line 24
> > <https://reviews.apache.org/r/33612/diff/2/?file=950466#file950466line24>
> >
> >     This will lock in our thrift version when these become final and seems 
> > brittle (easy to add a new field to Container and have it compile fine) - 
> > can you file a ticket to investigate alternatives to this pattern?

To be honest, i feel as though i already exhausted the options before getting 
here.  AFAICT the alternatives are non-trivial: change the thrift codegen and 
upgrade, or avoid use of thrift for database model objects.  I won't coerce you 
against filing a ticket, but i'd prefer not to since i doubt it will be 
addressed independently.


> On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 
> > 77
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line77>
> >
> >     Pull this CmdLine arg up to a module?

Ok


> On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 
> > 99
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line99>
> >
> >     Inject a Clock here?

Ok


> On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java,
> >  line 27
> > <https://reviews.apache.org/r/33612/diff/2/?file=950466#file950466line27>
> >
> >     `isSet(_Fields.DOCKER)`

Done.


> On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java,
> >  line 36
> > <https://reviews.apache.org/r/33612/diff/2/?file=950466#file950466line36>
> >
> >     Use isSet

Done.


> On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java,
> >  line 36
> > <https://reviews.apache.org/r/33612/diff/2/?file=950467#file950467line36>
> >
> >     isSet

Done.


> On May 12, 2015, 8:09 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java,
> >  line 27
> > <https://reviews.apache.org/r/33612/diff/2/?file=950467#file950467line27>
> >
> >     isSet

Done.


- Bill


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


On May 9, 2015, 5:53 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
> 
> (Updated May 9, 2015, 5:53 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-556
>     https://issues.apache.org/jira/browse/AURORA-556
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Add a task store implementation that uses a relational database.
> 
> The meat of this review is in `DbTaskStore`, `TaskConfigManager`, and the 
> mapper xml files.  Some supporting actors include files under views/, which 
> serve DB business objects.  I suggest reviewers start by skimming 
> `DbTaskStore` and digesting the comments in there.
> 
> There are some known loose ends here, most notably being continued 
> performance enhancements and cleanup of relations in different tables.  I've 
> included several relevant TODOs, ~all of which must be addressed before this 
> becomes the default task store.
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 0182ecb3942cfa2d9dd21138779815f4500339be 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> cc4864c4d954d58e9bfaaaf5fc5afc8d0e032e78 
>   src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java 
> 88d27dd729fd004d1510917a031591addba51816 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 239c61625bc49e53be918c59056f071b3b6b86b9 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 
> ea5600725d5dd84d21ca8d37b560c6d41541d016 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> c8df88b77b9a2017c48bdc0c9a0477927ba0b179 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 1a6c3f21d4fcb476539f588facecd8ef37332837 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskMapper.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/ScheduleStatusTypeHandler.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java
>  4d0c10d60037a3310226a6fd8936b84ae4135e7e 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/AssignedPort.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/ScheduledTaskWrapper.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskConfigRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskLink.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/mem/InMemStoresModule.java 
> 21f7d4db821930d2c5b52648e1996aa1ef12a85c 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
>  f76f9a988669dab598e9d19f849018c6f55ce03e 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/TaskMapper.xml 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> afb7db8eefa63b84d370877742870acdec58899c 
>   
> src/test/java/org/apache/aurora/scheduler/storage/AbstractTaskStoreTest.java 
> 6a6ff27d8d0f1e537a74c1df981b97e5d8b2f2e8 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  7d856d020da854c125c037f01357e81de93895e1 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java 
> 8f139fc987a98ef0d7f2969720134729411b8b84 
>   
> src/test/java/org/apache/aurora/scheduler/storage/mem/StorageTransactionTest.java
>  bad9eb56b33c3e649c3b173e83d9c30da8f9317d 
> 
> Diff: https://reviews.apache.org/r/33612/diff/
> 
> 
> Testing
> -------
> 
> Unit tests and end-to-end tests, both using the new task store by default 
> with this change.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to