Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62606 --- Does this code have test coverage? I can't see any tests that validate chunking is performing in the intended way. - David McLaughlin On Nov. 21, 2014, 1:53 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 21, 2014, 1:53 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
On Nov. 21, 2014, 11:34 a.m., David McLaughlin wrote: Does this code have test coverage? I can't see any tests that validate chunking is performing in the intended way. Existing test coverage in LogManagerTest validates the on-disk chunked format is the same. This is only a refactor to stream the results as an `Iterablebyte[]` rather than buffer them as a `byte[][]`. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62606 --- On Nov. 20, 2014, 5:53 p.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 20, 2014, 5:53 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
On Nov. 21, 2014, 7:34 p.m., David McLaughlin wrote: Does this code have test coverage? I can't see any tests that validate chunking is performing in the intended way. Kevin Sweeney wrote: Existing test coverage in LogManagerTest validates the on-disk chunked format is the same. This is only a refactor to stream the results as an `Iterablebyte[]` rather than buffer them as a `byte[][]`. It seems like the only test that had to be updated was a test that accesses the first element. It's possible the other integration tests don't exceed the frame size or other conditions, right? - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62606 --- On Nov. 21, 2014, 1:53 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 21, 2014, 1:53 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
On Nov. 21, 2014, 7:34 p.m., David McLaughlin wrote: Does this code have test coverage? I can't see any tests that validate chunking is performing in the intended way. Kevin Sweeney wrote: Existing test coverage in LogManagerTest validates the on-disk chunked format is the same. This is only a refactor to stream the results as an `Iterablebyte[]` rather than buffer them as a `byte[][]`. David McLaughlin wrote: It seems like the only test that had to be updated was a test that accesses the first element. It's possible the other integration tests don't exceed the frame size or other conditions, right? This is a good question to ask. The test cases in LogManagerTest exercise this by chunking all log entries used in the test [1]. [1] https://github.com/apache/incubator-aurora/blob/253cb7370d6055c5b931a256f292b86973cbacfd/src/test/java/org/apache/aurora/scheduler/storage/log/LogManagerTest.java#L314-L330 - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62606 --- On Nov. 21, 2014, 1:53 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 21, 2014, 1:53 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62639 --- Ship it! Ship It! - David McLaughlin On Nov. 21, 2014, 1:53 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 21, 2014, 1:53 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62477 --- Master (ada97bd) is red with this patch. ./build-support/jenkins/build.sh + date Fri Nov 21 01:01:15 UTC 2014 + ./gradlew -Pq clean build :buildSrc:clean UP-TO-DATE :buildSrc:compileJava UP-TO-DATE :buildSrc:compileGroovy :buildSrc:processResources UP-TO-DATE :buildSrc:classes :buildSrc:jar :buildSrc:assemble :buildSrc:compileTestJava UP-TO-DATE :buildSrc:compileTestGroovy UP-TO-DATE :buildSrc:processTestResources UP-TO-DATE :buildSrc:testClasses UP-TO-DATE :buildSrc:test UP-TO-DATE :buildSrc:check UP-TO-DATE :buildSrc:build :clean :bootstrapThrift :checkPython :generateSources :compileGeneratedJavaNote: Some input files use unchecked or unsafe operations. Note: Recompile with -Xlint:unchecked for details. :processGeneratedResources UP-TO-DATE :generatedClasses :compileJavaNote: Writing file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2 :processResources :classes :jar :assemble :jsHint :checkstyleMain[ant:checkstyle] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java:93:13: Variable 'i' must be private and have accessor methods. FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':checkstyleMain'. Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.xml * Try: Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. BUILD FAILED Total time: 1 mins 29.996 secs I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 21, 2014, 12:54 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 21, 2014, 12:54 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62479 --- Ship it! src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java https://reviews.apache.org/r/28306/#comment104520 A generic RuntimeException would be better here. `NoSuchElementException` seems like a lie. - Bill Farner On Nov. 21, 2014, 12:54 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 21, 2014, 12:54 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62481 --- src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java https://reviews.apache.org/r/28306/#comment104522 Mind commenting on the algorithm here? Why the magic -2? - Maxim Khutornenko On Nov. 21, 2014, 12:54 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 21, 2014, 12:54 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 20, 2014, 5:10 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Changes --- Bill's + ReviewBot's feedback. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs (updated) - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62483 --- Master (ada97bd) is red with this patch. ./build-support/jenkins/build.sh File /usr/lib/python2.7/sysconfig.py, line 355, in _init_posix raise IOError(msg) IOError: invalid Python installation: unable to open /x1/jenkins/jenkins-slave/workspace/AuroraBot/build-support/pants.venv/local/include/python2.7/pyconfig.h (No such file or directory) Build operating on top level addresses: set([BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/BUILD, all)]) Problem executing PythonBuilder for targets OrderedSet([PythonTests(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/admin/BUILD, host_maintenance)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/BUILD, api)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, api)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, restarter)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, instance_watcher)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, scheduler_client)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/common/au th/BUILD, auth)), PythonThriftLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/thrift/org/apache/aurora/gen/BUILD, py-thrift)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/common/BUILD, cluster)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/common/BUILD, transport)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, scheduler_mux)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, error_handling_thread)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, task_util)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/BUILD, base)), PythonLibrary (BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/common/BUILD, http_signaler)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, updater_util)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, sla)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/common/BUILD, common)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/common/BUILD, aurora_job_key)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/common/BUILD, cluster_option)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/common/BUILD, clusters)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/ workspace/AuroraBot/src/main/python/apache/aurora/common/BUILD, shellify)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/thrift/org/apache/aurora/gen/BUILD, py-thrift-packaged)), PythonThriftLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/thrift/org/apache/aurora/gen/BUILD, py-thrift-test)), PythonThriftLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/thrift/org/apache/aurora/gen/BUILD, py-thrift-storage)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, updater)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, job_monitor)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/client/api/BUILD, quota_check)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace /AuroraBot/src/main/python/apache/aurora/admin/BUILD, host_maintenance)), PythonLibrary(BuildFileAddress(/x1/jenkins/jenkins-slave/workspace/AuroraBot/src/main/python/apache/aurora/admin/BUILD, util)),
Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62484 --- @ReviewBot retry - Kevin Sweeney On Nov. 20, 2014, 5:10 p.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 20, 2014, 5:10 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
On Nov. 20, 2014, 5:09 p.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java, line 92 https://reviews.apache.org/r/28306/diff/1/?file=771713#file771713line92 Mind commenting on the algorithm here? Why the magic -2? Not sure what I'd say there that doesn't risk contradicting the code below - we don't usually comment loop variables and the old version had the same amount of commenting. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62481 --- On Nov. 20, 2014, 5:10 p.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 20, 2014, 5:10 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62493 --- Ship it! Master (ada97bd) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 21, 2014, 1:10 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 21, 2014, 1:10 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
On Nov. 21, 2014, 1:09 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java, line 92 https://reviews.apache.org/r/28306/diff/1/?file=771713#file771713line92 Mind commenting on the algorithm here? Why the magic -2? Kevin Sweeney wrote: Not sure what I'd say there that doesn't risk contradicting the code below - we don't usually comment loop variables and the old version had the same amount of commenting. I just find it harder to follow when I see anything less than -1 as the initial condition. Why not starting with -1 (to account for the header) and then increment i at the end of computeNext()? - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62481 --- On Nov. 21, 2014, 1:10 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 21, 2014, 1:10 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
On Nov. 20, 2014, 5:09 p.m., Maxim Khutornenko wrote:
src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java,
line 92
https://reviews.apache.org/r/28306/diff/1/?file=771713#file771713line92
Mind commenting on the algorithm here? Why the magic -2?
Kevin Sweeney wrote:
Not sure what I'd say there that doesn't risk contradicting the code
below - we don't usually comment loop variables and the old version had the
same amount of commenting.
Maxim Khutornenko wrote:
I just find it harder to follow when I see anything less than -1 as the
initial condition. Why not starting with -1 (to account for the header) and
then increment i at the end of computeNext()?
Each branch short-circuits so I'd have to have a result intermediate value. I
don't find that more readable
```java
byte[] result;
if (i == -1) {
result = header;
} else if (...) {
result = encode(...);
} else {
return endOfFile();
}
i++;
return result;
```
- Kevin
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28306/#review62481
---
On Nov. 20, 2014, 5:10 p.m., Kevin Sweeney wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28306/
---
(Updated Nov. 20, 2014, 5:10 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-930
https://issues.apache.org/jira/browse/AURORA-930
Repository: aurora
Description
---
Stream frame chunks instead of preallocating them.
This avoids allocating an entire additional snapshot worth of heap during
entry serialization, reducing overall heap impact of the serializer from
2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize.
Further optimizations out-of-scope for this change:
* Make the returned iterator mutate a fixed-size buffer (for GC pressure
avoidance).
* Change the log format so that FrameHeader doesn't need to know the size and
checksum of the serialized data ahead-of-time (maybe write it as a trailer).
Diffs
-
src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java
f4fa1cb740633ced529c1b5fd9f18abba8944571
src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java
cb95d8996a934751745f423b79279266d73b7722
src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java
c90389433d81dd72756c659736e38fd9f66fcb35
Diff: https://reviews.apache.org/r/28306/diff/
Testing
---
./gradlew -Pq build
Thanks,
Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
On Nov. 21, 2014, 1:09 a.m., Maxim Khutornenko wrote:
src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java,
line 92
https://reviews.apache.org/r/28306/diff/1/?file=771713#file771713line92
Mind commenting on the algorithm here? Why the magic -2?
Kevin Sweeney wrote:
Not sure what I'd say there that doesn't risk contradicting the code
below - we don't usually comment loop variables and the old version had the
same amount of commenting.
Maxim Khutornenko wrote:
I just find it harder to follow when I see anything less than -1 as the
initial condition. Why not starting with -1 (to account for the header) and
then increment i at the end of computeNext()?
Kevin Sweeney wrote:
Each branch short-circuits so I'd have to have a result intermediate
value. I don't find that more readable
```java
byte[] result;
if (i == -1) {
result = header;
} else if (...) {
result = encode(...);
} else {
return endOfFile();
}
i++;
return result;
```
I actually find it more readable as your if-else-if-else eliminates the need to
short-circuit. Anyway, just a nit that forced me to pause for awhile.
- Maxim
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28306/#review62481
---
On Nov. 21, 2014, 1:10 a.m., Kevin Sweeney wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28306/
---
(Updated Nov. 21, 2014, 1:10 a.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-930
https://issues.apache.org/jira/browse/AURORA-930
Repository: aurora
Description
---
Stream frame chunks instead of preallocating them.
This avoids allocating an entire additional snapshot worth of heap during
entry serialization, reducing overall heap impact of the serializer from
2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize.
Further optimizations out-of-scope for this change:
* Make the returned iterator mutate a fixed-size buffer (for GC pressure
avoidance).
* Change the log format so that FrameHeader doesn't need to know the size and
checksum of the serialized data ahead-of-time (maybe write it as a trailer).
Diffs
-
src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java
f4fa1cb740633ced529c1b5fd9f18abba8944571
src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java
cb95d8996a934751745f423b79279266d73b7722
src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java
c90389433d81dd72756c659736e38fd9f66fcb35
Diff: https://reviews.apache.org/r/28306/diff/
Testing
---
./gradlew -Pq build
Thanks,
Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62502 --- Ship it! Ship It! - Maxim Khutornenko On Nov. 21, 2014, 1:10 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 21, 2014, 1:10 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 20, 2014, 5:53 p.m.) Review request for Aurora, David McLaughlin and Bill Farner. Changes --- Maxim's feedback. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs (updated) - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
Re: Review Request 28306: Return an iterable of frame chunks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/#review62513 --- Ship it! Master (ada97bd) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 21, 2014, 1:53 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28306/ --- (Updated Nov. 21, 2014, 1:53 a.m.) Review request for Aurora, David McLaughlin and Bill Farner. Bugs: AURORA-930 https://issues.apache.org/jira/browse/AURORA-930 Repository: aurora Description --- Stream frame chunks instead of preallocating them. This avoids allocating an entire additional snapshot worth of heap during entry serialization, reducing overall heap impact of the serializer from 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize. Further optimizations out-of-scope for this change: * Make the returned iterator mutate a fixed-size buffer (for GC pressure avoidance). * Change the log format so that FrameHeader doesn't need to know the size and checksum of the serialized data ahead-of-time (maybe write it as a trailer). Diffs - src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java f4fa1cb740633ced529c1b5fd9f18abba8944571 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java cb95d8996a934751745f423b79279266d73b7722 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java c90389433d81dd72756c659736e38fd9f66fcb35 Diff: https://reviews.apache.org/r/28306/diff/ Testing --- ./gradlew -Pq build Thanks, Kevin Sweeney
