stoty commented on code in PR #5943: URL: https://github.com/apache/hbase/pull/5943#discussion_r1616669746
########## hbase-rest/src/main/java/org/apache/hadoop/hbase/rest/ProtobufMessageHandler.java: ########## @@ -18,21 +18,55 @@ package org.apache.hadoop.hbase.rest; import java.io.IOException; +import java.io.OutputStream; import org.apache.yetus.audience.InterfaceAudience; +import org.apache.hbase.thirdparty.com.google.protobuf.CodedOutputStream; +import org.apache.hbase.thirdparty.com.google.protobuf.Message; + /** * Common interface for models capable of supporting protobuf marshalling and unmarshalling. Hooks * up to the ProtobufMessageBodyConsumer and ProtobufMessageBodyProducer adapters. */ @InterfaceAudience.Private public interface ProtobufMessageHandler { - /** Returns the protobuf represention of the model */ - byte[] createProtobufOutput(); + + // The Jetty 9.4 HttpOutput default commit size is 32K/4 = 8K. We use that size to avoid + // double buffering (and copying) in HttpOutput. If we ever increase the HttpOutput commit size, + // we need to adjust this accordingly. We should also revisit this when Jetty is upgraded. + static final int BUFFER_SIZE = 8 * 1024; + + /** Writes the protobuf represention of the model */ + default void writeProtobufOutput(OutputStream os) throws IOException { + // Creating an explicit CodedOutputStream for the following reasons : + // 1. This avoids the cost of pre-computing the message size + // 2. This lets us set the buffer size explicitly + CodedOutputStream cos = CodedOutputStream.newInstance(os, BUFFER_SIZE); + messageFromObject().writeTo(cos); + cos.flush(); + } + + /** + * Use {@link org.apache.hadoop.hbase.rest#writeProtobufOutput(OutputStream)} with + * BufferedOutputStream for better performance + * @return the protobuf encoded object in a byte array + */ + default byte[] createProtobufOutput() { + return messageFromObject().toByteArray(); + } + Review Comment: While being marked as Interfacetype.PRIVATE, this is in fact used by all tests, and has to be used by client code as well. If we removed this, we'd have to change all protobuf tests, and all existing client code using protobuf would have to be rewritten. Ideally, we'd have some kind of public interface for marshalling/unmarshalling the REST structures client side, or we'd have them hooked up to the rest framework client code, but we don't. The existing REST Client class relies on passing marshalled byte arrays everywhere. There may be a way to provide a more elegant Jax-RS based client, but that would only apply to new clients. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org