----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64286/#review193751 -----------------------------------------------------------
Fix it, then Ship it! 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)? src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java Line 88 (original), 90 (patched) <https://reviews.apache.org/r/64286/#comment272392> We shouldn't map Edit -> op src/main/java/org/apache/aurora/scheduler/storage/durability/Loader.java Lines 53 (patched) <https://reviews.apache.org/r/64286/#comment272391> nit: op -> edit src/main/java/org/apache/aurora/scheduler/storage/durability/Persistence.java Lines 56 (patched) <https://reviews.apache.org/r/64286/#comment272394> 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? src/main/java/org/apache/aurora/scheduler/storage/durability/Persistence.java Lines 63-65 (patched) <https://reviews.apache.org/r/64286/#comment272389> Check if Op is not null here so people cannot accidentally create a `deleteAll` src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java Lines 100 (patched) <https://reviews.apache.org/r/64286/#comment272393> Do we need to map edit -> op? - Jordan Ly On Dec. 12, 2017, 5:46 p.m., Bill Farner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64286/ > ----------------------------------------------------------- > > (Updated Dec. 12, 2017, 5:46 p.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 > >