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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
