> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 43
> > <https://reviews.apache.org/r/21132/diff/3/?file=580759#file580759line43>
> >
> >     Breaking the abstraction by having a JobKeyMapper here is quite 
> > unfortunate. Does it mean we would have to add n mappers here if we add n 
> > new fields into the LockKey? Any way this could be generalized or 
> > abstracted out?
> 
> David McLaughlin wrote:
>     We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
>     
>     I don't see a way to create a storage abstraction for both in-memory and 
> relational databases without seeing artifacts like this in the 
> implementations. I think it's also made more complicated by this temporary 
> hybrid state we're in. 
>     
>
> 
> David McLaughlin wrote:
>     Just to expand on this. The key difference between just storing objects 
> on heap and having some formal storage layer like an RDBMS is that for 
> in-memory, the creation of the underlying entities is performed when I do 
> this:
>     
>     
>     LockKey.job(JobsKeys.from(role, env, job).builder());
>     
>     
>     For an RDBMS, we have to take this instance and fetch the existing 
> primary key from the table and/or write a new row to the DB. This logic 
> either lives inside the Storage.Mutable implementation, or it leaks out to 
> the application layer where it makes no sense for the existing on heap 
> solutions.
> 
> Maxim Khutornenko wrote:
>     | We would need to add n mappers if the we expand LockKey's fields. Most 
> likely we would change the design when that happens. 
>     Any thoughts on what that design would look like? I'd rather make an 
> effort to think it through now before locking ourselves into the schema.
>     
>     | LockKey.job(JobsKeys.from(role, env, job).builder());
>     Understood. However, this kind of specialized syntax is currently used 
> only on the application side (SchedulerThriftInterface) where it makes 
> perfect sense with the store handling it transparently.
>     
>     Setting aside this particular issue, my general concern is that we don't 
> seem to know how to handle thrift unions gracefully at this point. There are 
> a few of them in our .thrift files and I am hesitant to move forward without 
> a better story/solution around it.

My first take on this is that I'd have a manager/mapper class to represent the 
LockKey union, which is injected with all the mappers for the underlying types 
and handles the logic of running the correct underlying create statements based 
on which field is set in LockKey. It would look identical to the code now, just 
the type changes. 


- David


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


On May 13, 2014, 6:27 p.m., David McLaughlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21132/
> -----------------------------------------------------------
> 
> (Updated May 13, 2014, 6:27 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-335
>     https://issues.apache.org/jira/browse/AURORA-335
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Initial attempt at h2/DB storage implementation (LockStore only)
> 
> 
> Diffs
> -----
> 
>   build.gradle 6c758f690b87eede3ae3a7c54fabac20db543840 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> db1bec4f508c8908f212aa541fb86e041a8c471c 
>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
> 4b33fe5dd8223ff04060de0fe16b1c0759ead956 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/JobKeyMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/LockMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/MigrationModule.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/LockRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> c851eeb412b17097ff42abce2b7a42fc1c249013 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 
> 4d43f47de75a8bef06f106f563cc071df4cdf093 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java 
> a6319f65bff07db66197e6476647117df6be5c9d 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java 
> 283976ab0554dbe6700bb0d2a1b7702c969227e8 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java 
> 53923627c827131ee4bd93e5c4865d042aee501b 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/JobKeyMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/LockMapper.xml 
> PRE-CREATION 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> 34377430268002e8e8e5bc803b409e092bb86720 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java 
> a5191500b2958253e14843089a15a1ffd58e6846 
> 
> Diff: https://reviews.apache.org/r/21132/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David McLaughlin
> 
>

Reply via email to