[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user yisong-yue commented on the issue: https://github.com/apache/zookeeper/pull/690 couldn't reproduce this failure locally. seems like a flakey test case. retest this please ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user lvfangmin commented on the issue: https://github.com/apache/zookeeper/pull/690 retest this please ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user yisong-yue commented on the issue: https://github.com/apache/zookeeper/pull/690 ð Thanks for the feedback! ð ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/690 @yisong-yue Yeah, that's true. Perhaps it would be too much hassle and I'm trying to over-engineer things here. Let's just leave it as it is now and only do refactoring if we could benefit from it. ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user yisong-yue commented on the issue: https://github.com/apache/zookeeper/pull/690 @anmolnar One drawback I see with this structure is that it makes it harder to dynamically pick deserialization mode based on a snapshot's filename, since `FileSnap` manages a whole `snapDir` directory now. Maybe we should separate directory management logic from snapshot file (de)serialization in FileSnap class. ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/690 @yisong-yue Global state can still be maintained in a static field of a concrete class, it shouldn't be a problem. Though, you're probably right about `SnapStream` cannot maintain a state on its own, I believe because the methods should actually belong to `FileSnap`. Taking one step back and looking at how `FileSnap` works, I think - in terms of inheritance - the following would be the best to represent relationships: ``` SnapShot (interface) - represents a single snapshot of the DataTree | \\ _ FileSnap (abstract) - implements common methods for all file-based snapshots | \\ _ GzipFileSnap (concrete) - Gzip-compressed file-based snapshot | \\ _ SnappyFileSnap (concrete) - Snappy compressed file-based snapshot | ... ``` In this example, your new methods should go into the right concrete class whether it's Gzip, Snappy, etc. related or in the abstract `FileSnap` class if it's some common method. Of course, this needs to do a bit of a refactor which might not be trivial, so I wouldn't say we have to do this. I just wanted to give you an idea and get some feedback from you and the community. ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user yisong-yue commented on the issue: https://github.com/apache/zookeeper/pull/690 @anmolnar I agree that concrete classes are a lot easier to test. Though in this case, I think it's more appropriate for SnapStream to be a utility class that does not hold any state. (It contains a global, non-changing "state" `streamMode` now. It's a bit less ideal, but avoids having to pass in the same streamMode value every time.) For classes that need to hold some mutable state, I agree that concrete class is the way to go. ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user yisong-yue commented on the issue: https://github.com/apache/zookeeper/pull/690 Here are some benchmark results I did with my laptop: ``` (1) uncompressedsnappy gzip Size: ~ 15.9 MB ~ 4.1 MB ~ 0.3 MB Serialize (run 1): 110 ms 36 ms 134 ms Serialize (run 2): 96 ms 33 ms 135 ms Deserialize (run 1):40 ms 25 ms 76 ms Deserialize (run 2):39 ms 26 ms 74 ms (2) uncompressedsnappy gzip Size: ~ 382 MB~ 139 MB ~ 104 MB Serialize (run 1): 3393 ms 3279 ms 22096 ms Serialize (run 2): 3177 ms 3223 ms 24729 ms Deserialize (run 1):4239 ms 3532 ms 14122 ms Deserialize (run 2):5750 ms 4722 ms 16654 ms (3) uncompressedsnappy gzip Size: ~ 1.2 GB~ 560 MB ~ 330 MB Serialize (run 1): 24250 ms26575 ms208570 ms Serialize (run 2): 23717 ms25648 ms197400 ms Deserialize (run 1):51510 ms67274 ms 189142 ms Deserialize (run 2):50185 ms64691 ms 181521 ms ``` ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user yisong-yue commented on the issue: https://github.com/apache/zookeeper/pull/690 squashed two commits into one. ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/690 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2610/ --nonenone-- ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/690 @yisong-yue sorry for the spam. There is a problem with the pre-commit job. I won't disturb you anymore on this PR ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/690 retest this please ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/690 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2609/ --nonenone-- ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/690 retest this please ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/690 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2608/ ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user eolivelli commented on the issue: https://github.com/apache/zookeeper/pull/690 retest this please ---
[GitHub] zookeeper issue #690: ZOOKEEPER-3179: Add snapshot compression to reduce the...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/690 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2606/ ---
