aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kevin Sweeney" <kevi...@apache.org>
Subject Re: Review Request 25760: Performance improvements and instrumentation for snapshot
Date Thu, 18 Sep 2014 18:05:14 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message