----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26478/#review55986 -----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java <https://reviews.apache.org/r/26478/#comment96339> Method interceptors should work fine for package-priviate methods [1]. Just for my understanding - are you going with protected because package-private class + protected method is slightly more restrictive? [1] https://github.com/google/guice/wiki/AOP#limitations src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java <https://reviews.apache.org/r/26478/#comment96331> First sentence contains mixed thoughts for an odd sentence, rewrite? src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java <https://reviews.apache.org/r/26478/#comment96332> Please make this more verbose to explain what exactly is backwards in compatible. Don't worry about being too wordy. I'd also like to see a TODO to remove this flag, since it should become the default. Perhaps this should be included in the 0.7.0 deprecations list. src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotDeduplicator.java <https://reviews.apache.org/r/26478/#comment96334> Tasks.SCHEDULED_TO_INFO src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotDeduplicator.java <https://reviews.apache.org/r/26478/#comment96335> Do you have numbers on how much time this routine saves when compared to a full deep copy and unsetting the field you're trying to clear? Unless it's a significant contributor to overall snapshot performance, i'd prefer the more concise code of the latter approach. My hunch is that this one might save O(100 ms), but the ones below are noise and not worth the code. src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotDeduplicator.java <https://reviews.apache.org/r/26478/#comment96336> Inline this on :114. src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotDeduplicator.java <https://reviews.apache.org/r/26478/#comment96337> It would be nice to see a brief comment here to give the reader an overview of what's going on in the routine. src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotDeduplicator.java <https://reviews.apache.org/r/26478/#comment96338> remove newline src/main/thrift/org/apache/aurora/gen/storage.thrift <https://reviews.apache.org/r/26478/#comment96340> The mention of taskConfigId is vague. There should be a doc somewhere indicating that `taskConfigId` is referencing the _index_ of an entry in `taskConfigs`. src/main/thrift/org/apache/aurora/gen/storage.thrift <https://reviews.apache.org/r/26478/#comment96341> remove newline src/main/thrift/org/apache/aurora/gen/storage.thrift <https://reviews.apache.org/r/26478/#comment96342> Please doc. src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotDeduplicatorImplTest.java <https://reviews.apache.org/r/26478/#comment96343> The assertion error will be much more useful if you do `assertEquals(emptySet, b)` rather than `assertEquals(n, b.size())`. src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotDeduplicatorImplTest.java <https://reviews.apache.org/r/26478/#comment96344> Isn't this check redundant to the one below? src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotDeduplicatorImplTest.java <https://reviews.apache.org/r/26478/#comment96345> Can you try for `assertEquals` here as well, with an expected list of `DeduplicatedScheduledTask` objects? It will catch classes of bugs missed with this check (e.g. extra entries in the `partialTasks` list), and make it easier to diagnose a failed test. - Bill Farner On Oct. 9, 2014, 2:39 a.m., Kevin Sweeney wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26478/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2014, 2:39 a.m.) > > > Review request for Aurora, David McLaughlin, Bill Farner, and Zameer Manji. > > > Bugs: AURORA-722 > https://issues.apache.org/jira/browse/AURORA-722 > > > Repository: aurora > > > Description > ------- > > Add a new format for deduplicated storage snapshots. Microbenchmarks show a > 10x deduplication ratio on Twitter's production snapshots. > > This format is backwards-incompatible, so this patch introduces a flag to > control its use (defaulting off). > > This only changes the format used to write to the replicated log (where time > is of the essence since all writes are done holding the global storage lock) > - the format of backups written to disk is unchanged, as backups don't hold > the lock. > > > Diffs > ----- > > build.gradle 2e1bf78d7797f17afd51a94a22eff80e00aba464 > src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java > 65e986eaa2c4193431ca048425a1ed3ab60f5882 > src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java > 7239a6a5eb5479e395e16423c83fdf80a77e5a83 > src/main/java/org/apache/aurora/scheduler/storage/log/LogManager.java > 4b50e2069407dc263b4fc93f1827d3a8836253bf > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java > f806297d1d0700155c976743f936b2b8a3a390fb > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorageModule.java > 769348e6b8a5c701734afff391b1c77de35222c6 > > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotDeduplicator.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/storage/log/StreamManager.java > 22db80eaf34fe736fa5a3a9289836c9ac9e59906 > > src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java > e5cfbf5cf43bf5bbc38c42fe685a7e9f0d03af2a > src/main/thrift/org/apache/aurora/gen/storage.thrift > 5350ec945fbe028ee4641683815a068ce00b5efc > src/test/java/org/apache/aurora/scheduler/storage/log/LogManagerTest.java > 39729b374fe4e383f9b5ada7d016923766df9af7 > src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java > 7a8c3b882633376a1bf6a78616d55aaa7401d13f > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotDeduplicatorImplTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/26478/diff/ > > > Testing > ------- > > ./gradlew -Pq build > > > Thanks, > > Kevin Sweeney > >
