aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 21132: Initial attempt at h2/DB storage implementation (LockStore only)
Date Tue, 20 May 2014 19:08:08 GMT


> 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.
> 
> David McLaughlin wrote:
>     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. 
>
> 
> Maxim Khutornenko wrote:
>     Any chance we could see it implemented in this CR? It would set a precedent and give
us a clear path towards further thrift union migration.
> 
> Bill Farner wrote:
>     David and i discussed approaches to this in the first draft, and the 2 sane paths
forward we found were 1.) patch thrift to make it possible to subclass a codegen'd union class,
2.) tackle the problem with our own code generation to use different, custom types for database
models.  We decided that we would benefit from a short-term solution so we can vet the database
as a whole, and proceed with (2) as it becomes imminent (which it will, soon).

Sounds like a plan. David, would you mind adding a TODO describing the proposed approach (2)?


- Maxim


-----------------------------------------------------------
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
> 
>


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