You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Kevin Sweeney <ke...@apache.org> on 2014/09/18 02:20:19 UTC
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/
-----------------------------------------------------------
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
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
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
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
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
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
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
Posted by Kevin Sweeney <ke...@apache.org>.
> 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
Posted by Kevin Sweeney <ke...@apache.org>.
> 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()?
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."
> On Sept. 17, 2014, 5:56 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java, line 137
> > <https://reviews.apache.org/r/25760/diff/1/?file=693090#file693090line137>
> >
> > Static import?
done.
> On Sept. 17, 2014, 5:56 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/codec/ThriftBinaryCodec.java, line 171
> > <https://reviews.apache.org/r/25760/diff/1/?file=693090#file693090line171>
> >
> > Line break after '=' instead?
Looks like it fit on one line so this style question is avoided. For future style I'd advocate break after paren, since you can have multiple clauses in a try block, for example:
```java
try (
InputStream in = new FileInputStream(inFile);
OutputStream out = new FileOutputStream(outFile)) {
ByteStreams.copy(in, out);
} catch (IOException e) {
// ...
}
```
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25760/#review53778
-----------------------------------------------------------
On Sept. 17, 2014, 5:20 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25760/
> -----------------------------------------------------------
>
> (Updated Sept. 17, 2014, 5:20 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
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
>
>