aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 33612: Add a task store implementation that uses a relational database.
Date Sat, 09 May 2015 17:52:23 GMT


> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, lines 207-208
> > <https://reviews.apache.org/r/33612/diff/2/?file=950460#file950460line207>
> >
> >     Doesn't need to be javadoc.

Filled out with a comment.


> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, lines 102-106
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line102>
> >
> >     It'd be nice if this was extracted out to a higher level so it was reusable
across DB store impls without needing to be repeated in each method.
> >     
> >     Maybe replace the usage of the TimedInterceptor with a new DBTimedInterceptor
or some such?
> >     
> >     (If you think this is a good idea I'm fine with just dropping a TODO and revisiting
this in a follow up).

Unless you feel strongly, i would like to punt.  I'm not too keen on building out abstractions
for multiple store implementations, since multiple implementations is intended to be temporary.


> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, lines 169-174
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line169>
> >
> >     Would it make sense to use a LoadingCache for this to simplify it a bit, rather
than explicitly inserting on a cache miss, we could rely on the LoadingCache to either return
the existing config id or insert one and return the new id?

I went down this path because of the runtime checks offered by `BiMap`, but you've convinced
me that it's not worth it.  Switched to LoadingCache, thanks for the suggestion!


> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 264
> > <https://reviews.apache.org/r/33612/diff/2/?file=950462#file950462line264>
> >
> >     nit: kill else

Done.


> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java, line
127
> > <https://reviews.apache.org/r/33612/diff/2/?file=950463#file950463line127>
> >
> >     This seems like it may be important to have (without it we may leave stale data
behind)? Are you comfortable shipping this without properly cleaning up?

I'm not comfortable using it in production as-is, but wanted to draw the line somewhere w.r.t.
patch size.

I'd also like to have a scoped discussion about what our strategy should be here, since it's
a pattern that applies to several other tables.  The review addressing this TODO will be a
great forum for that discussion.


> On May 7, 2015, 10:22 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/TaskMapper.java, line 30
> > <https://reviews.apache.org/r/33612/diff/2/?file=950465#file950465line30>
> >
> >     I'm totally unclear on our guidelines for javadoc. Usually on interface methods
we'd require javadoc, does that not hold here for some reason since it's a MyBatis mapper?

Added docs.  TBH i'm not certain they're valuable in this context, but we have them on other
mappers so i will choose to conform.


- Bill


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


On May 5, 2015, 6:21 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> -----------------------------------------------------------
> 
> (Updated May 5, 2015, 6:21 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 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message