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

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. 


> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 50
> > <https://reviews.apache.org/r/21132/diff/3/?file=580759#file580759line50>
> >
> >     This is exactly what I was afraid to see. What if the lock key is not 
> > an IJobKey? Would it require a switch on type to determine the right insert 
> > path?

Well, that switch would always need to happen for a union greater than one 
element.

But you are right here and above that I took some shortcuts and went for a 
simpler implementation by relying on the fact that the thrift union is actually 
just one type. I left a comment on this somewhere, but can add more docs if 
that helps. 


> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbLockStore.java, line 
> > 77
> > <https://reviews.apache.org/r/21132/diff/3/?file=580759#file580759line77>
> >
> >     Can we use LockRow on the insertion path to solve the problem above?

LockRow wouldn't really solve the underlying problem - that none of our Thrift 
types have a field to store the primary keys on the tables, so DbLockStore 
doesn't have all the information it needs to resolve the relationship. So 
*somewhere* you need take a LockRow/ILock/LockKey and ensure it exists in the 
DB before trying to save the lock (see the insert SQL in LockMapper.xml for the 
weirdness we need to do to work around this). 

Ultimately I think it makes a lot of sense to encapsulate this logic and the 
error handling in DbLockStore, rather than push off that responsibility to the 
application layer. 


> On May 16, 2014, 4 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql, lines 
> > 10-19
> > <https://reviews.apache.org/r/21132/diff/3/?file=580773#file580773line10>
> >
> >     Inline with my above comments, this implementation will not scale when 
> > adding new LockKey types. Is it possible to have a join table between locks 
> > and job_keys to abstract out the dependency? 
> >     
> >     Something like: 
> >     locks(..., FK:lock_key_id)
> >     lock_keys(lock_key_id, lock_type, FK:lock_type_id -> job_key_id)
> >     job_keys(job_key_id, ...)

That looks like an unnecessary intermediate table to me? You could have the 
exact same effect by inlining those columns into the locks table. 


- 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