Add CompressibleBytesOutputStream for compression #27540
Conversation
This is a follow-up to elastic#23941. Currently there are a number of complexities related to compression. The raw DeflaterOutputStream must be closed prior to sending bytes to ensure that EOS bytes are written. But the underlying ReleasableBytesStreamOutput cannot be closed until the bytes are sent to ensure that the bytes are not reused. Right now we have three different stream references hanging around in TCPTransport to handle this complexity. This commit introduces CompressibleBytesOutputStream to be one stream implemenation that will behave properly with or without compression enabled.
|
To be clear about #27525, the issue there has nothing to do with compressible streams but rather handling closing of the releasable bytes output stream when an exception is thrown before we attach the send listener that would otherwise close the stream. The reason that #24927 addresses this though is due to how it simplifies closing of the releasable bytes output stream whether or not the send listener is attached. |
| } | ||
|
|
||
| @Override | ||
| public void reset() throws IOException { |
There was a problem hiding this comment.
In practice it probably does not matter, I think we will never call reset on these streams.
If the stream is not compressed, then resetting is fine.
If the stream is compressed, then we would already throw an unsupported operation exception (from OutputStreamStreamOutput#reset).
There was a problem hiding this comment.
I think it would be good to be consistent and always throw
| public void close() { | ||
| if (stream == bytesStreamOutput) { | ||
| assert shouldCompress == false : "If the streams are the same we should not be compressing"; | ||
| IOUtils.closeWhileHandlingException(stream); |
There was a problem hiding this comment.
why do we not bubble up the exception here?
There was a problem hiding this comment.
I think this is an artifact of the compressible bytes output stream implementing releasable which does not declare any checked exceptions. I think we should remove this and let these bubble up as you say. I opened: #27542
There was a problem hiding this comment.
this PR still doesn't bubble up exceptions?!
There was a problem hiding this comment.
| public void close() { | ||
| if (stream == bytesStreamOutput) { | ||
| assert shouldCompress == false : "If the streams are the same we should not be compressing"; | ||
| IOUtils.closeWhileHandlingException(stream); |
There was a problem hiding this comment.
this PR still doesn't bubble up exceptions?!
s1monw
left a comment
s1monw
left a comment
There was a problem hiding this comment.
thanks for the extra iters
|
I pushed this change and backported the other changes that we discussed:
|
jasontedor
added
:Distributed Coordination/Network
3 participants
This is a follow-up to #23941. Currently there are a number of complexities related to compression. The raw DeflaterOutputStream must be closed prior to sending bytes to ensure that EOS bytes are written. But the underlying ReleasableBytesStreamOutput cannot be closed until the bytes are sent to ensure that the bytes are not reused.
Right now we have three different stream references hanging around in TCPTransport to handle this complexity. This commit introduces CompressibleBytesOutputStream to be one stream implemenation that will behave properly with or without compression enabled.
This is a backport of #24927 to 5.6.
Closes #27525