Directly compressing StreamOutput

[DaveCTurner]

Today Compressor#threadLocalOutputStream returns a bare (buffered)
OutputStream which many callers then adapt into a StreamOutput using
the inefficient OutputStreamStreamOutput. This commit adjusts the
compressor to return a BufferedStreamOutput, avoiding the need for an
extra wrapper while also supporting more efficient structured write
operations such as writeVInt and writeString.

[elasticsearchmachine]

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

[joshua-adams-1]

LGTM

[joshua-adams-1]

I'm surprised no tests needed updating? I would have assumed there was a test for the branch where this try block failed

[DaveCTurner]

I don't follow. This is just removing the now-unnecessary OutputStreamStreamOutput wrapper, there's no change in failure handling or anything here is there?

[joshua-adams-1]

The original code was:

try (var bytesStream = transportService.newNetworkBytesStream()) {
    try (
        var stream = new OutputStreamStreamOutput(
            CompressorFactory.COMPRESSOR.threadLocalOutputStream(Streams.flushOnCloseStream(bytesStream))
        )
    ) {
        stream.setTransportVersion(version);
        clusterState.writeTo(stream);
    } catch (IOException e) {
        throw new ElasticsearchException("failed to serialize cluster state for publishing to node {}", e, discoveryNode);
    }
    ...

I would have expected there to be a test for the ElasticsearchException that used mocklog or equivalent to throw an exception when the OutputStreamStreamOutput was created. Since we've changed this now to:

var stream = CompressorFactory.COMPRESSOR.threadLocalStreamOutput(Streams.flushOnCloseStream(bytesStream)))

I suspected that any test similar to the above would need to be updated. Checking JoinValidationServiceTests here yields no test for this code branch

[DaveCTurner]

Creating an OutputStreamStreamOutput throws no exceptions?

[joshua-adams-1]

My mistake - I didn't check the constructor for OutputStreamStreamOutput. I didn't realise this try/catch block was only for auto-closable, I thought it might be protecting against an IOException too.

As a note, I still don't think the ElasticsearchException is tested, since I cannot find any reference to it in the JoinValidationServiceTests, nor the message failed to serialize cluster state for publishing to node anywhere else in the Elasticsearch codebase, but that isn't relevant to this change

[joshua-adams-1]

This is a change of behaviour. Can it be documented in a Javadoc please? (Either at the method or class level)

[DaveCTurner]

This is actually the expected behaviour of close() - see java.io.Closeable#close. Turns out we hardly use or test that fact.

[joshua-adams-1]

Ah, interesting! Is this worth adding to a github issue or other relevant documentation to track this testing gap?

[DaveCTurner]

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.