> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 131
> > <https://reviews.apache.org/r/33612/diff/1/?file=944387#file944387line131>
> >
> >     This feels like a potential for flakiness. Why 5? Any reason against 
> > setting it to -1 to let instances die with the tests?

-1 would cause the DBs to stick around until the JVM exits, and we'd accrue one 
for every test **case**.  I share your concern of flakiness, though.  An 
alternative would be to push `NonVolatileStorage.stop()` up to 
`VolatileStorage`, and establish the pattern of invoking that during tear down. 
 Does that sound better to you?


> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 
> > 154
> > <https://reviews.apache.org/r/33612/diff/1/?file=944389#file944389line154>
> >
> >     One perf improvement here could be avoiding deleting TaskConfigs as 
> > they are going to be re-inserted right away by the following loop (a very 
> > likely scenario for something like 'killall'). In fact, I don't see how 
> > `saveTasks` could require deleting any configs at all. Perhaps leave a TODO 
> > to carve out a `deleteTasksWithoutConfigs` method?

This would break behavior alignment with MemTaskStore, which i would really 
prefer to avoid at this phase (`saveTasks()` can technically be used as an 
upsert).


> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java,
> >  line 119
> > <https://reviews.apache.org/r/33612/diff/1/?file=944390#file944390line119>
> >
> >     I'd vote for explicit control. If `deleteTasks` is not called from 
> > `saveTasks` (see my comment above), the only caller is going to be the 
> > TaskHistoryPruner. Given that's a background task, perf impact here is less 
> > of an issue.

Yeah, i plan to have this discussion more formally, as i'd like to agree on the 
pattern.  I'm much less worried about performance than correctness - managing 
all these reference counts is going to be a sizeable chore.


> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml,
> >  line 302
> > <https://reviews.apache.org/r/33612/diff/1/?file=944402#file944402line302>
> >
> >     This seems unrelated to this diff.

Would you prefer i open a new review?  I used this pattern in another mapper in 
the diff, and wanted to align on style.


> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml,
> >  lines 161-163
> > <https://reviews.apache.org/r/33612/diff/1/?file=944403#file944403line161>
> >
> >     Would it make sense to explore collection sub-selects instead? Having 
> > executor_data repeated many times per match could potentially damage perf. 
> > If you agree, please leave a TODO.

I agree that it's reasonably likely to be an outcome, but i'd like to avoid 
dropping too many perf TODOs at this point so as to let the benchmarks speak 
for themselves.


> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java, 
> > line 27
> > <https://reviews.apache.org/r/33612/diff/1/?file=944407#file944407line27>
> >
> >     Can you add tests (or TODOs) for all other TaskStore methods?

You mean in addition to those provided in `AbstractTaskStoreTest`?


- Bill


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


On April 28, 2015, 8:11 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
> 
> (Updated April 28, 2015, 8:11 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 
> 82ad42fd0a626672dca80a5362fc07d804b3e412 
>   src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java 
> ed1171d52655fef643330f34913c256f77300fa2 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 3d19831ea0eb85032172b096ac87e126701aa218 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 
> 94ce5c3499ced1b63abf19787acc21b2cd4d0c75 
>   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 
> e3b13407cb6875489e50cf93212845eab7aacb89 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java 
> f18619a27eeb2aea8dcf01e54c23ed7d1c7d3d87 
>   
> 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