You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2015/05/20 23:47:25 UTC
Review Request 34501: Defaulting TemporaryStorage to in-memory task
store.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34501/
-----------------------------------------------------------
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
Re: Review Request 34501: Defaulting TemporaryStorage to in-memory
task store.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34501/#review84598
-----------------------------------------------------------
Ship it!
Master (998993d) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
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
>
>
Re: Review Request 34501: Enabling TemporaryStorage to use flagged
task store.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34501/#review85040
-----------------------------------------------------------
Ship it!
Master (6db13ba) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On May 23, 2015, 12:31 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34501/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 12:31 a.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Bugs: AURORA-1322
> https://issues.apache.org/jira/browse/AURORA-1322
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Allowing TemporaryStorage to use command line controlled task store.
>
>
> Diffs
> -----
>
> src/jmh/java/org/apache/aurora/benchmark/SnapshotBenchmarks.java 03dd5d99b269a1add709504b21fc197fdef9e18c
> src/jmh/java/org/apache/aurora/benchmark/UpdateStoreBenchmarks.java f361caa6f4e3b28f4a1e4d57f1989a6aa6fe258e
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 23c0c1e73a183be748199610ddf03e5d654fef74
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 436d3841b9361df4db98a2217e61abb95e6e6bab
> src/main/java/org/apache/aurora/scheduler/storage/db/DbUtil.java fe8e3f8f164732769fa0ae50a62e89c8aa77e9a5
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java 177d720b59ba601d59aada9650aba799babb9a73
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java 999d5e893897dc24659ca4a240ad3d4615cac0e5
>
> Diff: https://reviews.apache.org/r/34501/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> Manual restore from backup in Vagrant.
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 34501: Enabling TemporaryStorage to use flagged
task store.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34501/#review85449
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Sweeney
On May 22, 2015, 5:31 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34501/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 5:31 p.m.)
>
>
> Review request for Aurora and Kevin Sweeney.
>
>
> Bugs: AURORA-1322
> https://issues.apache.org/jira/browse/AURORA-1322
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Allowing TemporaryStorage to use command line controlled task store.
>
>
> Diffs
> -----
>
> src/jmh/java/org/apache/aurora/benchmark/SnapshotBenchmarks.java 03dd5d99b269a1add709504b21fc197fdef9e18c
> src/jmh/java/org/apache/aurora/benchmark/UpdateStoreBenchmarks.java f361caa6f4e3b28f4a1e4d57f1989a6aa6fe258e
> src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 23c0c1e73a183be748199610ddf03e5d654fef74
> src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 436d3841b9361df4db98a2217e61abb95e6e6bab
> src/main/java/org/apache/aurora/scheduler/storage/db/DbUtil.java fe8e3f8f164732769fa0ae50a62e89c8aa77e9a5
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java 177d720b59ba601d59aada9650aba799babb9a73
> src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java 999d5e893897dc24659ca4a240ad3d4615cac0e5
>
> Diff: https://reviews.apache.org/r/34501/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
> Manual restore from backup in Vagrant.
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 34501: Enabling TemporaryStorage to use flagged
task store.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34501/
-----------------------------------------------------------
(Updated May 23, 2015, 12:31 a.m.)
Review request for Aurora and Kevin Sweeney.
Changes
-------
Kevin's comments.
Summary (updated)
-----------------
Enabling TemporaryStorage to use flagged task store.
Bugs: AURORA-1322
https://issues.apache.org/jira/browse/AURORA-1322
Repository: aurora
Description (updated)
-------
Allowing TemporaryStorage to use command line controlled task store.
Diffs (updated)
-----
src/jmh/java/org/apache/aurora/benchmark/SnapshotBenchmarks.java 03dd5d99b269a1add709504b21fc197fdef9e18c
src/jmh/java/org/apache/aurora/benchmark/UpdateStoreBenchmarks.java f361caa6f4e3b28f4a1e4d57f1989a6aa6fe258e
src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java 23c0c1e73a183be748199610ddf03e5d654fef74
src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 436d3841b9361df4db98a2217e61abb95e6e6bab
src/main/java/org/apache/aurora/scheduler/storage/db/DbUtil.java fe8e3f8f164732769fa0ae50a62e89c8aa77e9a5
src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java 177d720b59ba601d59aada9650aba799babb9a73
src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java 999d5e893897dc24659ca4a240ad3d4615cac0e5
Diff: https://reviews.apache.org/r/34501/diff/
Testing
-------
./gradlew -Pq build
Manual restore from backup in Vagrant.
Thanks,
Maxim Khutornenko
Re: Review Request 34501: Defaulting TemporaryStorage to in-memory
task store.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
>
>
Re: Review Request 34501: Defaulting TemporaryStorage to in-memory
task store.
Posted by Kevin Sweeney <ke...@apache.org>.
> On May 20, 2015, 3: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.
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.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34501/#review84601
-----------------------------------------------------------
On May 20, 2015, 2: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, 2: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
>
>
Re: Review Request 34501: Defaulting TemporaryStorage to in-memory
task store.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
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.
- 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
>
>
Re: Review Request 34501: Defaulting TemporaryStorage to in-memory
task store.
Posted by Kevin Sweeney <ke...@apache.org>.
> On May 20, 2015, 3: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".
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?
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34501/#review84601
-----------------------------------------------------------
On May 20, 2015, 2: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, 2: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
>
>
Re: Review Request 34501: Defaulting TemporaryStorage to in-memory
task store.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
>
>
Re: Review Request 34501: Defaulting TemporaryStorage to in-memory
task store.
Posted by Kevin Sweeney <ke...@apache.org>.
> On May 20, 2015, 3: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.
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.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34501/#review84601
-----------------------------------------------------------
On May 20, 2015, 2: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, 2: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
>
>
Re: Review Request 34501: Defaulting TemporaryStorage to in-memory
task store.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34501/#review84601
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
<https://reviews.apache.org/r/34501/#comment135932>
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
On May 20, 2015, 2: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, 2: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
>
>