> 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? > > Bill Farner wrote: > -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?
An explicit teardown seems more predictable and easy to reason. I recognize the ovehead it may bring to unit tests though, so I wonder what others think. > 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? > > Bill Farner wrote: > 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). It's still not clear to me how upsert of a scheduled task can ever require deleting any task configs. It's an additive operation by definition that cannot possiblty render any TaskConfigs irrelevant. > 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. > > Bill Farner wrote: > Would you prefer i open a new review? I used this pattern in another > mapper in the diff, and wanted to align on style. Up to you. I was expecting to see some job update relevant changes when I spot this file though. > 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? > > Bill Farner wrote: > You mean in addition to those provided in `AbstractTaskStoreTest`? Aha, makes sense, I did not think we had these. - Maxim ----------------------------------------------------------- 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 > >
