> 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.
> 
> Maxim Khutornenko wrote:
>     > 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".
> 
> Kevin Sweeney wrote:
>     I'm not sure I understand your concern regarding the point about using a 
> factory binding. Could inject a `@Temporary Provider<Storage>` and implement 
> a `@Provides` method that does exactly what you describe. The code that wants 
> the production storage instance would still consume the `Singleton`-scoped 
> `Storage`. The implementation can be, essentially, `DbUtil.createStorage()` 
> that respects the beta flag.
>     
>     Regarding the concern about messed up data at the logical level that 
> seems like a (hopefully unlikely) risk of running with something tagged 
> "beta" turned up. Disabling that codepath entirely would have prevented you 
> from discovering this bug at all. Since it seems like we've reached a 
> deadlock on that point, would you mind adding another committer to the people 
> line for another opinion?

I am still uneasy about defaulting to a flagged storage for recovery but I am 
going to accept your suggestion to unblock this diff as there is still a way to 
use in-memory storage. I am going to forgo the injection suggestion though as 
it would create yet another way to initialize DB storage, which is already way 
too cluttered. Refactored testModule() use cases to reduce the call pattern 
variety.


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

Reply via email to