Re: Review Request 25760: Performance improvements and instrumentation for snapshot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25760/ --- (Updated Sept. 18, 2014, 11:13 a.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Changes --- Maxim's feedback Bugs: AURORA-722 https://issues.apache.org/jira/browse/AURORA-722 Repository: aurora Description --- * Deflate snapshots using stream API * Make LogManager non-final Diffs (updated) - src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java da4f0e5520f85782bbc246752cde29bd279be466 src/main/java/org/apache/aurora/scheduler/storage/log/Entries.java 1eca768a7daf6defd4bb35a5c46e9c0eab370d92 src/main/java/org/apache/aurora/scheduler/storage/log/LogManager.java 0b59043cf5f01c99d09168d7669f51b686d2e930 src/test/java/org/apache/aurora/codec/ThriftBinaryCodecTest.java 7aaab1e7debbfed65ac7c15154ec73fc9f2114af Diff: https://reviews.apache.org/r/25760/diff/ Testing --- ./gradlew -Pq build The StreamManager @Timed annotations don't work yet since guice isn't used to instantiate it. Ideally we'd use AssistedInject here, but that's a slightly larger change. Thanks, Kevin Sweeney
Re: Review Request 25760: Performance improvements and instrumentation for snapshot
On Sept. 18, 2014, 12:56 a.m., Maxim Khutornenko wrote:
src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java, line 152
https://reviews.apache.org/r/25760/diff/1/?file=693090#file693090line152
Technically, the outBytes may already be disposed at this point. Move
return inside of outer try()?
Kevin Sweeney wrote:
This is intentional since doing it outside the block means .close() will
have been called on the deflater output stream, flusing any buffers.
outBytes won't have been disposed because it's still in scope. From
http://docs.oracle.com/javase/7/docs/api/java/io/ByteArrayOutputStream.html#close()
Closing a ByteArrayOutputStream has no effect. The methods in this class
can be called after the stream has been closed without generating an
IOException.
FWIW, your ByteArrayOutputStream is already flushed as closing the transport
will also close its inner output stream. BTW, you may want to move
transport.close() into its own finally{} to cover the exception case.
- Maxim
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25760/#review53778
---
On Sept. 18, 2014, 6:13 p.m., Kevin Sweeney wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25760/
---
(Updated Sept. 18, 2014, 6:13 p.m.)
Review request for Aurora, Maxim Khutornenko and Bill Farner.
Bugs: AURORA-722
https://issues.apache.org/jira/browse/AURORA-722
Repository: aurora
Description
---
* Deflate snapshots using stream API
* Make LogManager non-final
Diffs
-
src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java
da4f0e5520f85782bbc246752cde29bd279be466
src/main/java/org/apache/aurora/scheduler/storage/log/Entries.java
1eca768a7daf6defd4bb35a5c46e9c0eab370d92
src/main/java/org/apache/aurora/scheduler/storage/log/LogManager.java
0b59043cf5f01c99d09168d7669f51b686d2e930
src/test/java/org/apache/aurora/codec/ThriftBinaryCodecTest.java
7aaab1e7debbfed65ac7c15154ec73fc9f2114af
Diff: https://reviews.apache.org/r/25760/diff/
Testing
---
./gradlew -Pq build
The StreamManager @Timed annotations don't work yet since guice isn't used to
instantiate it. Ideally we'd use AssistedInject here, but that's a slightly
larger change.
Thanks,
Kevin Sweeney
Re: Review Request 25760: Performance improvements and instrumentation for snapshot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25760/ --- (Updated Sept. 18, 2014, 12:45 p.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Changes --- Fix bad merge. Bugs: AURORA-722 https://issues.apache.org/jira/browse/AURORA-722 Repository: aurora Description --- * Deflate snapshots using stream API * Make LogManager non-final Diffs (updated) - src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java da4f0e5520f85782bbc246752cde29bd279be466 src/main/java/org/apache/aurora/scheduler/storage/log/Entries.java 1eca768a7daf6defd4bb35a5c46e9c0eab370d92 src/main/java/org/apache/aurora/scheduler/storage/log/LogManager.java 0b59043cf5f01c99d09168d7669f51b686d2e930 src/test/java/org/apache/aurora/codec/ThriftBinaryCodecTest.java 7aaab1e7debbfed65ac7c15154ec73fc9f2114af Diff: https://reviews.apache.org/r/25760/diff/ Testing --- ./gradlew -Pq build The StreamManager @Timed annotations don't work yet since guice isn't used to instantiate it. Ideally we'd use AssistedInject here, but that's a slightly larger change. Thanks, Kevin Sweeney
Re: Review Request 25760: Performance improvements and instrumentation for snapshot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25760/ --- (Updated Sept. 18, 2014, 12:57 p.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Changes --- Remove unneeded try-with-resources blocks Bugs: AURORA-722 https://issues.apache.org/jira/browse/AURORA-722 Repository: aurora Description --- * Deflate snapshots using stream API * Make LogManager non-final Diffs (updated) - src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java da4f0e5520f85782bbc246752cde29bd279be466 src/main/java/org/apache/aurora/scheduler/storage/log/Entries.java 1eca768a7daf6defd4bb35a5c46e9c0eab370d92 src/main/java/org/apache/aurora/scheduler/storage/log/LogManager.java 0b59043cf5f01c99d09168d7669f51b686d2e930 src/test/java/org/apache/aurora/codec/ThriftBinaryCodecTest.java 7aaab1e7debbfed65ac7c15154ec73fc9f2114af Diff: https://reviews.apache.org/r/25760/diff/ Testing --- ./gradlew -Pq build The StreamManager @Timed annotations don't work yet since guice isn't used to instantiate it. Ideally we'd use AssistedInject here, but that's a slightly larger change. Thanks, Kevin Sweeney
Re: Review Request 25760: Performance improvements and instrumentation for snapshot
On Sept. 17, 2014, 5:56 p.m., Maxim Khutornenko wrote:
src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java, line 152
https://reviews.apache.org/r/25760/diff/1/?file=693090#file693090line152
Technically, the outBytes may already be disposed at this point. Move
return inside of outer try()?
Kevin Sweeney wrote:
This is intentional since doing it outside the block means .close() will
have been called on the deflater output stream, flusing any buffers.
outBytes won't have been disposed because it's still in scope. From
http://docs.oracle.com/javase/7/docs/api/java/io/ByteArrayOutputStream.html#close()
Closing a ByteArrayOutputStream has no effect. The methods in this class
can be called after the stream has been closed without generating an
IOException.
Maxim Khutornenko wrote:
FWIW, your ByteArrayOutputStream is already flushed as closing the
transport will also close its inner output stream. BTW, you may want to move
transport.close() into its own finally{} to cover the exception case.
Since TIOStreamTransport calls close for me and propagates IOException as
TException I dropped by own handling of that.
- Kevin
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25760/#review53778
---
On Sept. 18, 2014, 12:57 p.m., Kevin Sweeney wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25760/
---
(Updated Sept. 18, 2014, 12:57 p.m.)
Review request for Aurora, Maxim Khutornenko and Bill Farner.
Bugs: AURORA-722
https://issues.apache.org/jira/browse/AURORA-722
Repository: aurora
Description
---
* Deflate snapshots using stream API
* Make LogManager non-final
Diffs
-
src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java
da4f0e5520f85782bbc246752cde29bd279be466
src/main/java/org/apache/aurora/scheduler/storage/log/Entries.java
1eca768a7daf6defd4bb35a5c46e9c0eab370d92
src/main/java/org/apache/aurora/scheduler/storage/log/LogManager.java
0b59043cf5f01c99d09168d7669f51b686d2e930
src/test/java/org/apache/aurora/codec/ThriftBinaryCodecTest.java
7aaab1e7debbfed65ac7c15154ec73fc9f2114af
Diff: https://reviews.apache.org/r/25760/diff/
Testing
---
./gradlew -Pq build
The StreamManager @Timed annotations don't work yet since guice isn't used to
instantiate it. Ideally we'd use AssistedInject here, but that's a slightly
larger change.
Thanks,
Kevin Sweeney
Re: Review Request 25760: Performance improvements and instrumentation for snapshot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25760/#review53778 --- Ship it! src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java https://reviews.apache.org/r/25760/#comment93582 Static import? src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java https://reviews.apache.org/r/25760/#comment93581 Technically, the outBytes may already be disposed at this point. Move return inside of outer try()? src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java https://reviews.apache.org/r/25760/#comment93583 Line break after '=' instead? src/test/java/org/apache/aurora/codec/ThriftBinaryCodecTest.java https://reviews.apache.org/r/25760/#comment93585 Now you have to add jsmith to reviewers :) - Maxim Khutornenko On Sept. 18, 2014, 12:20 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25760/ --- (Updated Sept. 18, 2014, 12:20 a.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Bugs: AURORA-722 https://issues.apache.org/jira/browse/AURORA-722 Repository: aurora Description --- * Deflate snapshots using stream API * Make LogManager non-final Diffs - src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java da4f0e5520f85782bbc246752cde29bd279be466 src/main/java/org/apache/aurora/scheduler/storage/log/Entries.java 1eca768a7daf6defd4bb35a5c46e9c0eab370d92 src/main/java/org/apache/aurora/scheduler/storage/log/LogManager.java 0b59043cf5f01c99d09168d7669f51b686d2e930 src/test/java/org/apache/aurora/codec/ThriftBinaryCodecTest.java 7aaab1e7debbfed65ac7c15154ec73fc9f2114af Diff: https://reviews.apache.org/r/25760/diff/ Testing --- ./gradlew -Pq build The StreamManager @Timed annotations don't work yet since guice isn't used to instantiate it. Ideally we'd use AssistedInject here, but that's a slightly larger change. Thanks, Kevin Sweeney
Re: Review Request 25760: Performance improvements and instrumentation for snapshot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25760/#review53786 --- Ship it! LGTM mod Maxim's comments. - Bill Farner On Sept. 18, 2014, 12:20 a.m., Kevin Sweeney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25760/ --- (Updated Sept. 18, 2014, 12:20 a.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Bugs: AURORA-722 https://issues.apache.org/jira/browse/AURORA-722 Repository: aurora Description --- * Deflate snapshots using stream API * Make LogManager non-final Diffs - src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java da4f0e5520f85782bbc246752cde29bd279be466 src/main/java/org/apache/aurora/scheduler/storage/log/Entries.java 1eca768a7daf6defd4bb35a5c46e9c0eab370d92 src/main/java/org/apache/aurora/scheduler/storage/log/LogManager.java 0b59043cf5f01c99d09168d7669f51b686d2e930 src/test/java/org/apache/aurora/codec/ThriftBinaryCodecTest.java 7aaab1e7debbfed65ac7c15154ec73fc9f2114af Diff: https://reviews.apache.org/r/25760/diff/ Testing --- ./gradlew -Pq build The StreamManager @Timed annotations don't work yet since guice isn't used to instantiate it. Ideally we'd use AssistedInject here, but that's a slightly larger change. Thanks, Kevin Sweeney
