> On Dec. 13, 2017, 6:07 p.m., Jordan Ly wrote: > > Small nits but overall LGTM. In addition to the end-to-end tests passing, > > could you also do a sanity check that this patch works fine on a small > > cluster (upgrade to this patch and downgrade to the previous version, > > ensuring snapshots will read by both fine)?
Happy to do this, i'll hold off committing until confirmed. > On Dec. 13, 2017, 6:07 p.m., Jordan Ly wrote: > > src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java > > Line 88 (original), 90 (patched) > > <https://reviews.apache.org/r/64286/diff/3/?file=1914592#file1914592line90> > > > > We shouldn't map Edit -> op Loader accepts `Stream<Edit>` and `Snapshotter#asStream()` produces `Stream<Op>`, so it's necessary to translate types. Maybe i'm misunderstanding you? > On Dec. 13, 2017, 6:07 p.m., Jordan Ly wrote: > > src/main/java/org/apache/aurora/scheduler/storage/durability/Persistence.java > > Lines 56 (patched) > > <https://reviews.apache.org/r/64286/diff/3/?file=1914595#file1914595line56> > > > > This seems like a very specialized abstraction so we can pass a storage > > reset. Not an issue at the moment, but do you have any additional plans for > > this class in the future? It's entirely for this purpose. I'm open to suggestions on this one. > On Dec. 13, 2017, 6:07 p.m., Jordan Ly wrote: > > src/main/java/org/apache/aurora/scheduler/storage/durability/Persistence.java > > Lines 63-65 (patched) > > <https://reviews.apache.org/r/64286/diff/3/?file=1914595#file1914595line63> > > > > Check if Op is not null here so people cannot accidentally create a > > `deleteAll` Yikes, thanks! > On Dec. 13, 2017, 6:07 p.m., Jordan Ly wrote: > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java > > Lines 100 (patched) > > <https://reviews.apache.org/r/64286/diff/3/?file=1914611#file1914611line117> > > > > Do we need to map edit -> op? To line up the types, yes. Same as above - unless i'm misunderstanding you. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64286/#review193751 ----------------------------------------------------------- On Dec. 12, 2017, 9:46 a.m., Bill Farner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64286/ > ----------------------------------------------------------- > > (Updated Dec. 12, 2017, 9:46 a.m.) > > > Review request for Aurora, Jordan Ly and Stephan Erb. > > > Repository: aurora > > > Description > ------- > > This cleans up the various interfaces around persisting and recovering from > `Snapshot`s. Most importantly, `LogPersistence` no longer bypasses the > `recover()` `Op` stream to apply snapshots. As a result, it should be > straightforward to build a migration utility that clones `LogPersistence` > state into another `Persistence` implementation. > > **Reviewers** > Apologies for the large patch. These components were circular, and i did not > find a clean way to break the important pieces of this change apart. > > > Diffs > ----- > > src/jmh/java/org/apache/aurora/benchmark/SnapshotBenchmarks.java > 755582da0e6ae9fcbfb191c195f86a003d00eb2a > > src/main/java/org/apache/aurora/scheduler/storage/DistributedSnapshotStore.java > 0c6a955279f4cadac87c765d91602ac307162f7d > src/main/java/org/apache/aurora/scheduler/storage/SnapshotStore.java > 6b5e5dd5b4d1e28f6d604bcc87c983453574adbf > src/main/java/org/apache/aurora/scheduler/storage/Snapshotter.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/storage/backup/BackupModule.java > 7eaae89d63bf45d6ddc3cffd8ee62d4549e0cc10 > src/main/java/org/apache/aurora/scheduler/storage/backup/Recovery.java > 3a62f02212971f1f73933018ac11702a7041b7d4 > src/main/java/org/apache/aurora/scheduler/storage/backup/StorageBackup.java > 2d61678c76d350286415265fef5056d5377d9788 > > src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java > 18296b0d9674c05ce4b6b36dbcdc060bc2d17b0a > > src/main/java/org/apache/aurora/scheduler/storage/durability/DurableStorage.java > 6a7c0add489ab1599fabe676e73582d575aa8c6e > src/main/java/org/apache/aurora/scheduler/storage/durability/Loader.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/durability/Persistence.java > 9eb862c01bf451252bfbcc7a2eac60d2c965c9f0 > > src/main/java/org/apache/aurora/scheduler/storage/durability/WriteAheadStorage.java > 667db062e791a916853edb7fabd6235a2e9bc272 > src/main/java/org/apache/aurora/scheduler/storage/log/LogPersistence.java > e70e6051582ca90ae72014626b983bbf4b8d5b48 > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java > 75ec42aad0b822d6c3dcd5b1307a4fcb86caa5c0 > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotService.java > PRE-CREATION > > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java > 739fad75c4d569a7f2ec01875a4ae63893ed7ac8 > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > 159fb29cd575f1d7bdba2fe0c98ae5381097a451 > src/test/java/org/apache/aurora/scheduler/app/local/LocalSchedulerMain.java > aeb8685e4e2d65c70636965a3a615e426c7a5ec2 > src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java > 5cb5310ed096ca1fb47b980401e3712948271ac4 > src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java > 09560f4929a252568ed487fbf27aac5a83e62c79 > > src/test/java/org/apache/aurora/scheduler/storage/backup/StorageBackupTest.java > fff376fabba37ea450ade469e48c5e85d6b3f96a > > src/test/java/org/apache/aurora/scheduler/storage/durability/DurableStorageTest.java > 3ad40adf58128ff41c5b88e3c5a7814b139e7d50 > > src/test/java/org/apache/aurora/scheduler/storage/durability/WriteAheadStorageTest.java > e8b564b0c7fb82d4057f6ee8dcf7b327f1c5e18c > > src/test/java/org/apache/aurora/scheduler/storage/log/LogPersistenceTest.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/storage/log/NonVolatileStorageTest.java > eb966d722dc01d1760566bc57358afac722d5fec > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotServiceTest.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java > 5634f921cc3d9b1773af0a7ab5a5150756501df3 > > src/test/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java > fd81bff5315d290f2b2c36959d566d5c595ef9c0 > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > 919ac145d96d75ec7f117784eea0ccfea8a2210e > src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java > bb0fd89b9edee2048746876da9edbaf5c9e881ca > > > Diff: https://reviews.apache.org/r/64286/diff/3/ > > > Testing > ------- > > end-to-end tests pass > > > Thanks, > > Bill Farner > >