-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Directly compressing StreamOutput
#140502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,6 +37,7 @@ public class BufferedStreamOutput extends StreamOutput { | |
| private final int endPosition; | ||
| private int position; | ||
| private long flushedBytes; | ||
| private boolean isClosed; | ||
|
|
||
| /** | ||
| * Wrap the given stream, using the given {@link BytesRef} for the buffer. It is the caller's responsibility to make sure that nothing | ||
|
|
@@ -129,8 +130,11 @@ private boolean assertTrashBuffer() { | |
|
|
||
| @Override | ||
| public void close() throws IOException { | ||
| flush(); | ||
| delegate.close(); | ||
| if (isClosed == false) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a change of behaviour. Can it be documented in a Javadoc please? (Either at the method or class level)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually the expected behaviour of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, interesting! Is this worth adding to a github issue or other relevant documentation to track this testing gap?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we'd ever get around to acting on such an issue tbh, it's too broad. I think it's enough to have assertions etc in the places where it actually matters. |
||
| isClosed = true; | ||
| flush(); | ||
| delegate.close(); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised no tests needed updating? I would have assumed there was a test for the branch where this
tryblock failedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow. This is just removing the now-unnecessary
OutputStreamStreamOutputwrapper, there's no change in failure handling or anything here is there?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code was:
I would have expected there to be a test for the
ElasticsearchExceptionthat used mocklog or equivalent to throw an exception when theOutputStreamStreamOutputwas created. Since we've changed this now to:I suspected that any test similar to the above would need to be updated. Checking
JoinValidationServiceTestshere yields no test for this code branchThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating an
OutputStreamStreamOutputthrows no exceptions?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake - I didn't check the constructor for
OutputStreamStreamOutput. I didn't realise thistry/catchblock was only for auto-closable, I thought it might be protecting against anIOExceptiontoo.As a note, I still don't think the
ElasticsearchExceptionis tested, since I cannot find any reference to it in theJoinValidationServiceTests, nor the messagefailed to serialize cluster state for publishing to nodeanywhere else in the Elasticsearch codebase, but that isn't relevant to this change