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 34501: Defaulting TemporaryStorage to in-memory task store.
Date Wed, 20 May 2015 23:33:05 GMT


> On May 20, 2015, 10:17 p.m., Kevin Sweeney wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java,
lines 72-76
> > <https://reviews.apache.org/r/34501/diff/1/?file=965743#file965743line72>
> >
> >     How is the db storage going to graduate to production if it's not actually used
when the beta flag is present?
> >     
> >     My preference is that we use whichever task store system is selected by the
command-line in all codepaths - if backup recovery fails here the cluster admin can relaunch
the scheduler with the H2 database disabled.
> 
> Kevin Sweeney wrote:
>     A better solution here IMO is to inject a TemporaryStorageFactory rather than call
the static `DbUtil.createStorage()` factory method directly. This allows us to configure the
choice of task store implementation in one place, rather than spread out across multiple packages,
which appears to have contributed to this issue.
> 
> Maxim Khutornenko wrote:
>     We have to provide a solid/guaranteed way to recover from failure. Chances are very
high we are in recover because of the TaskStore messing up our data. Relying on a DB task
store is not a solution we can rely on in such cases. In fact, the reason I filed this bug
was exactly that - I was not able to load an external backup due to DB task store violating
some schema constraints.
>     
>     > How is the db storage going to graduate to production if it's not actually used
when the beta flag is present?
>     
>     The graduation assumes removing the in-memory task store, right? At that moment we
will switch to a DB-based binding, which will become a default store.
> 
> Kevin Sweeney wrote:
>     Why is putting it behind a flag not sufficient? With your proposal we can't actually
test that DbStorage works for backup recovery until we make a release that supports only DbStorage.
If recovery fails with DbStorage because you have the `-enable_beta_storage` flag turned up,
then it seems completely reasonable to turn that flag down and try again.

> Why is putting it behind a flag not sufficient? 

If DB task store messes up data at the logical rather than schema level we will not be able
to guarantee the recovery until it could be too late to notice the problem.

> With your proposal we can't actually test that DbStorage works for backup recovery until
we make a release that supports only DbStorage. 

My point is that we don't have to. Once the beta DB store is ready to graduate, the task store
is going to be good enough to be used anywhere (including TemporaryStorage). 

BTW, we will still have to use a different binding to make sure the db instance is named something
other than "aurora".


- Maxim


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


On May 20, 2015, 9:47 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34501/
> -----------------------------------------------------------
> 
> (Updated May 20, 2015, 9:47 p.m.)
> 
> 
> Review request for Aurora and Kevin Sweeney.
> 
> 
> Bugs: AURORA-1322
>     https://issues.apache.org/jira/browse/AURORA-1322
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Defaulting TemporaryStorage to in-memory task store.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 23c0c1e73a183be748199610ddf03e5d654fef74

>   src/main/java/org/apache/aurora/scheduler/storage/db/DbUtil.java fe8e3f8f164732769fa0ae50a62e89c8aa77e9a5

>   src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java 177d720b59ba601d59aada9650aba799babb9a73

> 
> Diff: https://reviews.apache.org/r/34501/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> Manual restore from backup in Vagrant.
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>


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