aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bill Farner <wfar...@apache.org>
Subject Re: Review Request 62869: Exclusively use Map-based in-memory stores for primary storage
Date Wed, 11 Oct 2017 00:22:42 GMT


> On Oct. 10, 2017, 2:23 p.m., Stephan Erb wrote:
> > RELEASE-NOTES.md
> > Line 4 (original), 4 (patched)
> > <https://reviews.apache.org/r/62869/diff/1/?file=1851254#file1851254line4>
> >
> >     This is a major change. We should proabably call it out here.

Done.


> On Oct. 10, 2017, 2:23 p.m., Stephan Erb wrote:
> > api/src/main/thrift/org/apache/aurora/gen/storage.thrift
> > Line 149 (original)
> > <https://reviews.apache.org/r/62869/diff/1/?file=1851255#file1851255line149>
> >
> >     Can we remove that already? Could it be the case that we have some small scale
users that run using the DB task store and still need that migration?

_Disclaimer - please push back unless you're fully convinced_

This is not necessary; the field is only used to avoid redundant work.

On master:
- `Snapshot.tasks` and `Snapshot.cronJobs` will always be *written*
- `Snapshot.tasks` and `Snapshot.cronJobs` will be *read* if any of:
  a. CLI option `useDbSnapshotForTaskStore = false`
  b. `Snapshot.dbScript` is empty
  c. `Snapshot.experimentalTaskStore = false`

This patch will always populate `Snapshot.tasks` and `Snapshot.cronJobs`, and will never populate
`Snapshot.dbScript`.  As a result, the prior version will never skip over reading `Snapshot.tasks`
or `Snapshot.cronJobs`.


> On Oct. 10, 2017, 2:23 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java
> > Lines 47 (patched)
> > <https://reviews.apache.org/r/62869/diff/1/?file=1851266#file1851266line47>
> >
> >     Nitpick: Inconsistent style.

Fixed.


> On Oct. 10, 2017, 2:23 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
> > Lines 93-100 (original), 89-94 (patched)
> > <https://reviews.apache.org/r/62869/diff/1/?file=1851269#file1851269line94>
> >
> >     These comments are now outdated and should be removed.

Fixed, thanks!


> On Oct. 10, 2017, 2:23 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java
> > Lines 145-162 (original), 130-131 (patched)
> > <https://reviews.apache.org/r/62869/diff/1/?file=1851275#file1851275line149>
> >
> >     How about we leave this in but allow it to be disabled with a flag? 
> >     
> >     That way it would be easier to deploy this patch: If something goes wrong one
can simply perform a rollback as the snapshot is completely compatible. If however we stop
writing the db dump as done here, we would have to restore from a backup.
> >     
> >     We could remove this once we drop the entire H2/mybatis stuff.

> If however we stop writing the db dump as done here, we would have to restore from a
backup

Please see my previous comment about compatibility, but i believe this is incorrect.  I've
convinced myself that as long as `dbScript` is empty, all is well.  This is what motivated
doing all stores at once - to avoid piecemeal removal of tables from `dbScript`.


> On Oct. 10, 2017, 2:23 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java
> > Lines 246-249 (original), 217 (patched)
> > <https://reviews.apache.org/r/62869/diff/1/?file=1851275#file1851275line262>
> >
> >     To check my understanding: There are three cases we have to handle:
> >     
> >         a) db script but no host attributes in the snapshot -> we restore everything
from the db script 
> >         b) db script and host attributes in the snapshot -> the data is duplicated
but consistent. Restoring it twice will do no harm
> >         c) no db script any longer just host attributes in the snapshot: simple
restore as seen here. Will be the code path used in the future.
> >       
> >     Correct?

Correct, this is my understanding.


> On Oct. 10, 2017, 2:23 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/mem/MemQuotaStore.java
> > Lines 41 (patched)
> > <https://reviews.apache.org/r/62869/diff/1/?file=1851281#file1851281line41>
> >
> >     Argument checking for stores is somewhat inconsistent. We should decide if we
want them everywhere. If not, we should drop them.

Opting to remove based on popular majority within the code.


- Bill


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


On Oct. 10, 2017, 12:35 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62869/
> -----------------------------------------------------------
> 
> (Updated Oct. 10, 2017, 12:35 p.m.)
> 
> 
> Review request for Aurora, Jordan Ly and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> This patch introduces map-based volatile stores, most of which were revived from git
history with minimal changes.  The DB storage system is now only used in a temporary storage
when replaying a snapshot containing the `dbScript` field.
> 
> Please take special care to double-check my work in `SnapshotStoreImpl`, as it must function
for backwards compatibility with `Snapshot`s in the wild.  Another area of interest is the
implementation of `MemJobUpdateStore`, which must have nuanced behavior for compatibility
with `DbJobUpdateStore`.  Most of this stems from behind-the-scenes interaction with `LockStore`
and use of cascading deletes.
> 
> Note that this change removes the transactional nature of in-memory storage operations
as well as the `READ COMMITTED` transaction isolation previously available to some stores
(proven in necessary changes to `StorageTransactionTest`).  This means some stores will permit
dirty reads when they previously did not.  `TaskStore` has always had this non-transactional
behavior by default, as the DB task store was never deemed suitable for production.  Nonetheless,
this non-transactional behavior should be considered safe as the scheduler fails over on a
storage operation failure, and relies on the persistent log storage for transaction atomicity.
> 
> See the [mailing list discussion](https://lists.apache.org/thread.html/0bb74c78da7fa12954e2438e8011b3bff73fe3bfdafeaaa2ff00a98e@%3Cdev.aurora.apache.org%3E)
for context.
> 
> 
> Diffs
> -----
> 
>   RELEASE-NOTES.md c58e68080beb1740d92639601ef7cc29c63be37e 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 9ee9eff6c3d657decd93458dc3e6792a30614a60

>   docs/reference/scheduler-configuration.md 4e3f90713c307e3b9e9f84c29343af7f014f0165

>   examples/vagrant/upstart/aurora-scheduler.conf 63fcc87be653835cb3c3f25dae4164f1d7c8d4da

>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java cb783ce4e4b40f173fb3b1cbd0094a5a4e0300a5

>   src/jmh/java/org/apache/aurora/benchmark/SnapshotBenchmarks.java ae59f3def75d0e1d3866c8d2f85063456643e9a6

>   src/jmh/java/org/apache/aurora/benchmark/StateManagerBenchmarks.java b4f14f1f352dbe56afe57d66349ae5ae57533050

>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java c81387f24d554bcb2ee73e49028ba068ad11e4d6

>   src/jmh/java/org/apache/aurora/benchmark/TaskStoreBenchmarks.java 5b4d2a267b9cc1a14b915a1a10d63b4f4d174d51

>   src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java 440c4fc551905da513df5dae21d0eefe6a42ceeb

>   src/jmh/java/org/apache/aurora/benchmark/UpdateStoreBenchmarks.java cac02a55d26b2934099a2b03457c703600296292

>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java bb7055ed6826d57cb6b5c5c77e4a27c3bcdd10c7

>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 52c3c6618a3cf1009435ca8a9cece36365913e55

>   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 6c676693e531541ef9aec7f7a130c119ebf35df3

>   src/main/java/org/apache/aurora/scheduler/storage/Util.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java bad05f5bf1976bb590e8dd7af9b43d5d30e892a9

>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java cbe5a0deff1ebc38a9618e7d89ab073dfaf78d36

>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 0a2516e4843b8f920700ece70b3cc816d2acecf0

>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 7904e3880d214aac1013ce18265fed924ef7897e

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

>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java 835f1604c0c5d913a87d570ee01d053bbbf92ecb

>   src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java 9d2164f4f2d18e4595d3039d64cedacb7df00ae2

>   src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java d145259653dd4df90e3877fc3e744e24c7a15d13

>   src/main/java/org/apache/aurora/scheduler/storage/mem/InMemStoresModule.java 1b491f977cf3a81e61f1333082be0547420306d4

>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStore.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemJobUpdateStore.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemLockStore.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemQuotaStore.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemSchedulerStore.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/storage/mem/Util.java c28fb65010af5e3db925487929d4e0e12b4101a4

>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 54202359382bfe39e7cbaec0bf4c7d65d10ca13b

>   src/test/java/org/apache/aurora/scheduler/app/local/LocalSchedulerMain.java b63f014020ea9698d5869a92ca656823a923c21b

>   src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java 2b817076d6beb09586d4711bc10bceb31f3b74e1

>   src/test/java/org/apache/aurora/scheduler/cron/quartz/CronIT.java 8556253fc11f6027316871eb9dc66d8627a77fe6

>   src/test/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImplTest.java 81440f5689f9538a4c7a9e6700bf03ca89c4ba85

>   src/test/java/org/apache/aurora/scheduler/http/MaintenanceTest.java f94b58b77b7c6ce824914af7e1147e73ad5a7eed

>   src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java 09f443bca90f154b547d28ca5fd5be08177fdf99

>   src/test/java/org/apache/aurora/scheduler/state/LockManagerImplTest.java 19f9de312596395eff81bbfd073a5f617d2ef84c

>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 24176fe089703386e8246f6add1965c111c136ed

>   src/test/java/org/apache/aurora/scheduler/stats/ResourceCounterTest.java d73656dc8fb8764c7a66eed3ea789554ce0ecc52

>   src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java 04ec7715ba9a9d47c99d4ccb92939f3e561e1a60

>   src/test/java/org/apache/aurora/scheduler/storage/db/AttributeStoreTest.java PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/storage/db/DbAttributeStoreTest.java f47f4a8a492fb43bacd909dc520256ed028531dd

>   src/test/java/org/apache/aurora/scheduler/storage/db/DbCronJobStoreTest.java b68298ab50a3621da8596947619f2d1c34d2d194

>   src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java 8fca54becde34a0d10d60e05d3809fc1c41233bf

>   src/test/java/org/apache/aurora/scheduler/storage/db/DbLockStoreTest.java 8ed58e01eea09ab9f1aa4d269a5e59ce9c9c2191

>   src/test/java/org/apache/aurora/scheduler/storage/db/DbQuotaStoreTest.java 275d0fd93819169550ce39c8ddfa6c4fa0e4d920

>   src/test/java/org/apache/aurora/scheduler/storage/db/DbSchedulerStoreTest.java a6320bf687aa41c698bdc6a2c0b5208624cff4a3

>   src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java bcf5be43ab69ef565bb68f9235b4c43d385898aa

>   src/test/java/org/apache/aurora/scheduler/storage/db/JobUpdateStoreTest.java PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/storage/db/LockStoreTest.java PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/storage/db/QuotaStoreTest.java PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/storage/db/SchedulerStoreTest.java PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java 7f41430975d46440a404ac58395582f66fd902d4

>   src/test/java/org/apache/aurora/scheduler/storage/mem/InMemTaskStoreTest.java 02719c312294b58525c1fddd3ed096a9b1cef601

>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemAttributeStoreTest.java PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemCronJobStoreTest.java 26fe42992a7e1ae0c07b851383e9490141694972

>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemJobUpdateStoreTest.java PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemLockStoreTest.java PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemQuotaStoreTest.java PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemStorageTest.java PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/storage/mem/StorageTransactionTest.java 25f34e2bc26c6d4754c1591fad7f2165dd465d32

>   src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java 6b4b17f8dafd5c2d751dcda3080b476335f8aec0

>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 04a6e1e54a7ee61f1d35b3f5e9044236983fe019

> 
> 
> Diff: https://reviews.apache.org/r/62869/diff/2/
> 
> 
> Testing
> -------
> 
> Still need to complete (but should not block review):
> 
> * Post benchmark results (i've done these piecemeal, and as should be expected - things
are much faster)
> * Run `./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh` from scratch at this patch.
> * Run `./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh` on master, then this patch,
then on master.  This verifies some level of replay compatibility between versions.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>


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