> On Dec. 27, 2016, 11:50 p.m., Joshua Cohen wrote: > > Overall this looks good to me. I think some work would be required to > > productionize it (e.g. make it optional to start). Obviously want to vet > > this in a test cluster and we would need some doc changes to go with this > > and RELEASE-NOTES.md, etc. but that's easy enough to take care of. > > > > I also want to be sure I understand the behavior in the event of a problem > > with one, or both replicated logs. So... scenarios: > > > > 1) One log or the other is missing/corrupt: our state becomes the sum of > > whatever's found in the log that's present/valid. > > 2) There's no chance of a snapshot being persisted that references a log > > position that hasn't been persisted to the operation log, right? E.g. > > there's no realistic way that appending the noop transaction could take > > longer than creating/persisting the snapshot (in which case the snapshot > > could be successfully persisted referencing that position, yet that > > position would not exist in the operation log). > > David McLaughlin wrote: > 1) It's exactly the same as today if the log is corrupt - you need to > restore from a backup. > 2) No, that is not possible. Log appends are synchronous. > > I'm -1 to making this optional, purely because that doesn't reduce the > risk in any way - you'd just be putting off adding flags to your start-up > script. But yeah, it means I'll have to vet this change in a production-like > environment before this gets committed. > > Joshua Cohen wrote: > I'd like to hear what others running Aurora in production think about > putting this behind a flag. This feels like a big change to force on people > and I think if this came from someone outside of our org we'd like the > flexibility to enable this on our own timeframe rather than being blocked > from picking up all updates until we were comfortable/ready. > > Stephan Erb wrote: > Most people (us included) only deploy major versions. If we ship this in > 0.17 there will be many unrelated changes that might cause trouble and > require a rollback. So, even if this change is OK, people could be bitten by > other changes in the same release. > > I therefore agree with Joshua here that we need a way to make this > tansition smoother. > > David McLaughlin wrote: > A feature flag still doesn't solve that problem. You're just punting the > migration to another release that will also have commits that might cause a > rollback.
I still believe that a feature toggle would be helpful. I don't fear that this particular patch is broken, but rather that any of the hundreds of other changes within the release may come with a regression. Decoupling the backwards compatible from the incompatible parts just makes sure I can test the former before having to deal with the latter. An alternative solution would be to put this patch into a release of its own. - Stephan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54883/#review160188 ----------------------------------------------------------- On Dec. 22, 2016, 10:26 a.m., David McLaughlin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54883/ > ----------------------------------------------------------- > > (Updated Dec. 22, 2016, 10:26 a.m.) > > > Review request for Aurora, Joshua Cohen, John Sirois, Santhosh Kumar > Shanmugham, and Zameer Manji. > > > Repository: aurora > > > Description > ------- > > See here for more details: > https://docs.google.com/document/d/1QVSEfeoCyt2D6cCmTCxy8-epufcuzIfnqRUkyT1betY/edit?usp=sharing > > The motivation for this patch is the realisation that if we store Snapshots > outside of the main transaction log, we only ever to need to hold the storage > lock during snapshot creation. This would reduce the time writes are blocked > during snapshots by 80% for a large production cluster. I'm also doing this > with a view to future work, which is described in the document above. > > As long as everything works fine, this change is as simple as adding two new > args to the scheduler JVM. But it's worth noting that once this change is > deployed and the first snapshot is written, you will no longer be able to > rollback without first restoring from a backup. > > > Diffs > ----- > > api/src/main/thrift/org/apache/aurora/gen/storage.thrift > 9e4213f13255a182df938bea44ca87fa03a25318 > examples/vagrant/aurorabuild.sh dbec54d3e677db8cb0f02849e5373e619629fc7c > examples/vagrant/upstart/aurora-scheduler.conf > e68a801017ae02e0ed581129e12a645ccc1e35d4 > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 43cc5b4645bc26b0fc6b23726ad3292699048ded > src/main/java/org/apache/aurora/scheduler/log/Log.java > dc77eb435e5f8fdce56727a2f679e8e1907e977c > src/main/java/org/apache/aurora/scheduler/log/mesos/DualLogModule.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLog.java > 21855e184fe20dc339713978b32344b6950701ec > > src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java > 6704a328a4023a178ed8f86ae4772cb04eb2fa8e > src/main/java/org/apache/aurora/scheduler/storage/log/LogManager.java > cfa9c56c909bac2e885e33a9fe772cb41cbd9fa9 > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java > 387350c7667a5fb8ee674ad0d3dd17529232b25b > > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotDeduplicator.java > 9733ffe74b107f336858657550156ddb1f1dd215 > src/main/java/org/apache/aurora/scheduler/storage/log/StreamManager.java > ea147c0ba6aaa6d113144be0a8330bd2f73d2453 > > src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java > baf2647c54f1f9918139584e5151873a3853e83c > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java > 29a3b4a57925f31e59a49d4bfa630e724cadcb04 > src/test/java/org/apache/aurora/scheduler/log/mesos/MesosLogTest.java > f142f545799d64f9352b0ac6c51942eedf5e9ced > src/test/java/org/apache/aurora/scheduler/storage/log/LogManagerTest.java > 3f445595a81a5655c6c486791a9b55d8dc7f2f76 > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 34c24aac9339e59c435925bd2357ce86998f5f02 > > Diff: https://reviews.apache.org/r/54883/diff/ > > > Testing > ------- > > I currently have only tested this on one node (wrote this code on a flight). > Will have to verify this in our test clusters before shipping. > > > Example log output on my vagrant image: > > I1222 07:18:36.471 [qtp1117266705-37, LoggingInterceptor] snapshot() > I1222 07:18:36.479 [qtp1117266705-37, LogStorage] Creating snapshot at > position: Position 6 > I1222 07:18:36.485 [qtp1117266705-37, SnapshotStoreImpl] Saving dbsnapshot > I1222 07:18:36.591 [qtp1117266705-37, LogStorage] Write lock released. > I1222 07:18:36.591 [pool-12-thread-1, LogStorage] Persisting snapshot. > I1222 07:18:36.592 [pool-12-thread-1, > SnapshotDeduplicator$SnapshotDeduplicatorImpl] Starting deduplication of a > snapshot with 0 tasks. > I1222 07:18:36.595 [pool-12-thread-1, > SnapshotDeduplicator$SnapshotDeduplicatorImpl] Finished deduplicating > snapshot. Deduplication ratio: 0/0 = NaN%. > I1222 07:18:36.599218 4155 log.cpp:577] Attempting to append 6261 bytes to > the log > I1222 07:18:36.599321 4155 coordinator.cpp:348] Coordinator attempting to > write APPEND action at position 5 > I1222 07:18:36.599573 4160 replica.cpp:537] Replica received write request > for position 5 from (11)@192.168.33.7:8083 > I1222 07:18:36.600693 4160 leveldb.cpp:341] Persisting action (6280 bytes) > to leveldb took 1.061504ms > I1222 07:18:36.600733 4160 replica.cpp:712] Persisted action at 5 > I1222 07:18:36.600844 4160 replica.cpp:691] Replica received learned notice > for position 5 from @0.0.0.0:0 > I1222 07:18:36.601541 4160 leveldb.cpp:341] Persisting action (6282 bytes) > to leveldb took 619657ns > I1222 07:18:36.601569 4160 replica.cpp:712] Persisted action at 5 > I1222 07:18:36.601582 4160 replica.cpp:697] Replica learned APPEND action at > position 5 > I1222 07:18:36.603044 4160 log.cpp:596] Attempting to truncate the log to 5 > I1222 07:18:36.603173 4160 coordinator.cpp:348] Coordinator attempting to > write TRUNCATE action at position 6 > I1222 07:18:36.603330 4160 replica.cpp:537] Replica received write request > for position 6 from (12)@192.168.33.7:8083 > I1222 07:18:36.603890 4160 leveldb.cpp:341] Persisting action (16 bytes) to > leveldb took 502063ns > I1222 07:18:36.603919 4160 replica.cpp:712] Persisted action at 6 > I1222 07:18:36.603991 4160 replica.cpp:691] Replica received learned notice > for position 6 from @0.0.0.0:0 > I1222 07:18:36.604279 4160 leveldb.cpp:341] Persisting action (18 bytes) to > leveldb took 217834ns > I1222 07:18:36.604357 4160 leveldb.cpp:399] Deleting ~3 keys from leveldb > took 13629ns > I1222 07:18:36.604372 4160 replica.cpp:712] Persisted action at 6 > I1222 07:18:36.604383 4160 replica.cpp:697] Replica learned TRUNCATE action > at position 6 > I1222 07:18:36.604581 4160 log.cpp:596] Attempting to truncate the log to 6 > I1222 07:18:36.604682 4160 coordinator.cpp:348] Coordinator attempting to > write TRUNCATE action at position 7 > I1222 07:18:36.604778 4160 replica.cpp:537] Replica received write request > for position 7 from (13)@192.168.33.7:8083 > I1222 07:18:36.605317 4160 leveldb.cpp:341] Persisting action (16 bytes) to > leveldb took 485544ns > I1222 07:18:36.605356 4160 replica.cpp:712] Persisted action at 7 > I1222 07:18:36.605439 4160 replica.cpp:691] Replica received learned notice > for position 7 from @0.0.0.0:0 > I1222 07:18:36.605736 4160 leveldb.cpp:341] Persisting action (18 bytes) to > leveldb took 215503ns > I1222 07:18:36.605773 4160 leveldb.cpp:399] Deleting ~3 keys from leveldb > took 9620ns > I1222 07:18:36.605787 4160 replica.cpp:712] Persisted action at 7 > I1222 07:18:36.605798 4160 replica.cpp:697] Replica learned TRUNCATE action > at position 7 > I1222 07:18:36.605 [pool-12-thread-1, LogStorage] Snapshot complete. host > attrs: 1, cron jobs: 0, locks: 0, quota confs: 0, tasks: 0, updates: 0 > > > Thanks, > > David McLaughlin > >
