Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
stoty merged PR #5943: URL: https://github.com/apache/hbase/pull/5943 -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
ankitsinghal commented on code in PR #5943:
URL: https://github.com/apache/hbase/pull/5943#discussion_r1615407245
##
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:
Now this will not be used anywhere? Should we remove it?
--
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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132858928 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 32s | Docker mode activated. | | -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 54s | master passed | | +1 :green_heart: | compile | 0m 17s | master passed | | +1 :green_heart: | shadedjars | 5m 39s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 12s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 53s | the patch passed | | +1 :green_heart: | compile | 0m 17s | the patch passed | | +1 :green_heart: | javac | 0m 17s | the patch passed | | +1 :green_heart: | shadedjars | 5m 37s | patch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 13s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | unit | 3m 32s | hbase-rest in the patch passed. | | | | 23m 0s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/4/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux 256831636a68 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Eclipse Adoptium-17.0.10+7 | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/4/testReport/ | | Max. process+thread count | 1773 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/4/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132857034 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 11s | Docker mode activated. | | -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 45s | master passed | | +1 :green_heart: | compile | 0m 19s | master passed | | +1 :green_heart: | shadedjars | 5m 14s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 16s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 50s | the patch passed | | +1 :green_heart: | compile | 0m 19s | the patch passed | | +1 :green_heart: | javac | 0m 19s | the patch passed | | +1 :green_heart: | shadedjars | 5m 11s | patch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 15s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | unit | 3m 42s | hbase-rest in the patch passed. | | | | 22m 0s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/4/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux c15d5f963d40 5.4.0-182-generic #202-Ubuntu SMP Fri Apr 26 12:29:36 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Eclipse Adoptium-11.0.17+8 | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/4/testReport/ | | Max. process+thread count | 1493 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/4/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132854853 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 9s | Docker mode activated. | ||| _ Prechecks _ | | +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. | | +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. | | +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 58s | master passed | | +1 :green_heart: | compile | 0m 29s | master passed | | +1 :green_heart: | checkstyle | 0m 12s | master passed | | +1 :green_heart: | spotless | 0m 44s | branch has no errors when running spotless:check. | | +1 :green_heart: | spotbugs | 0m 29s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 45s | the patch passed | | +1 :green_heart: | compile | 0m 28s | the patch passed | | +1 :green_heart: | javac | 0m 28s | the patch passed | | +1 :green_heart: | checkstyle | 0m 11s | the patch passed | | +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. | | +1 :green_heart: | hadoopcheck | 4m 50s | Patch does not cause any errors with Hadoop 3.3.6. | | +1 :green_heart: | spotless | 0m 42s | patch has no errors when running spotless:check. | | +1 :green_heart: | spotbugs | 0m 36s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | asflicense | 0m 12s | The patch does not generate ASF License warnings. | | | | 20m 45s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/4/artifact/yetus-general-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile | | uname | Linux ffc31be4f6a3 5.4.0-182-generic #202-Ubuntu SMP Fri Apr 26 12:29:36 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Eclipse Adoptium-11.0.17+8 | | Max. process+thread count | 79 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/4/console | | versions | git=2.34.1 maven=3.8.6 spotbugs=4.7.3 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132856127 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 30s | Docker mode activated. | | -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 38s | master passed | | +1 :green_heart: | compile | 0m 17s | master passed | | +1 :green_heart: | shadedjars | 5m 10s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 15s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 27s | the patch passed | | +1 :green_heart: | compile | 0m 18s | the patch passed | | +1 :green_heart: | javac | 0m 18s | the patch passed | | +1 :green_heart: | shadedjars | 5m 8s | patch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 14s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | unit | 3m 27s | hbase-rest in the patch passed. | | | | 21m 28s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/4/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux 234ecde8b4c0 5.4.0-182-generic #202-Ubuntu SMP Fri Apr 26 12:29:36 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Temurin-1.8.0_352-b08 | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/4/testReport/ | | Max. process+thread count | 1628 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/4/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132816153 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 34s | Docker mode activated. | | -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 3m 12s | master passed | | +1 :green_heart: | compile | 0m 17s | master passed | | +1 :green_heart: | shadedjars | 5m 37s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 14s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 53s | the patch passed | | +1 :green_heart: | compile | 0m 17s | the patch passed | | +1 :green_heart: | javac | 0m 17s | the patch passed | | +1 :green_heart: | shadedjars | 5m 39s | patch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 12s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | unit | 3m 28s | hbase-rest in the patch passed. | | | | 23m 19s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux 09e1fb23c1a9 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Eclipse Adoptium-17.0.10+7 | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/testReport/ | | Max. process+thread count | 1738 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132813818 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 13s | Docker mode activated. | | -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 43s | master passed | | +1 :green_heart: | compile | 0m 19s | master passed | | +1 :green_heart: | shadedjars | 5m 11s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 16s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 47s | the patch passed | | +1 :green_heart: | compile | 0m 19s | the patch passed | | +1 :green_heart: | javac | 0m 19s | the patch passed | | +1 :green_heart: | shadedjars | 5m 11s | patch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 15s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | unit | 3m 50s | hbase-rest in the patch passed. | | | | 21m 53s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux a792ba989614 5.4.0-182-generic #202-Ubuntu SMP Fri Apr 26 12:29:36 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Eclipse Adoptium-11.0.17+8 | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/testReport/ | | Max. process+thread count | 1454 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132812198 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 31s | Docker mode activated. | | -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 23s | master passed | | +1 :green_heart: | compile | 0m 18s | master passed | | +1 :green_heart: | shadedjars | 5m 7s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 14s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 27s | the patch passed | | +1 :green_heart: | compile | 0m 17s | the patch passed | | +1 :green_heart: | javac | 0m 17s | the patch passed | | +1 :green_heart: | shadedjars | 5m 6s | patch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 13s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | unit | 3m 28s | hbase-rest in the patch passed. | | | | 20m 56s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux 4273f4a82286 5.4.0-182-generic #202-Ubuntu SMP Fri Apr 26 12:29:36 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Temurin-1.8.0_352-b08 | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/testReport/ | | Max. process+thread count | 1588 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132811970 :broken_heart: **-1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 9s | Docker mode activated. | ||| _ Prechecks _ | | +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. | | +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. | | +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 57s | master passed | | +1 :green_heart: | compile | 0m 29s | master passed | | +1 :green_heart: | checkstyle | 0m 12s | master passed | | +1 :green_heart: | spotless | 0m 45s | branch has no errors when running spotless:check. | | +1 :green_heart: | spotbugs | 0m 30s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 46s | the patch passed | | +1 :green_heart: | compile | 0m 28s | the patch passed | | +1 :green_heart: | javac | 0m 28s | the patch passed | | -0 :warning: | checkstyle | 0m 11s | hbase-rest: The patch generated 1 new + 23 unchanged - 0 fixed = 24 total (was 23) | | +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. | | +1 :green_heart: | hadoopcheck | 4m 52s | Patch does not cause any errors with Hadoop 3.3.6. | | -1 :x: | spotless | 0m 41s | patch has 24 errors when running spotless:check, run spotless:apply to fix. | | +1 :green_heart: | spotbugs | 0m 37s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | asflicense | 0m 9s | The patch does not generate ASF License warnings. | | | | 20m 43s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/artifact/yetus-general-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile | | uname | Linux 76a353861a84 5.4.0-182-generic #202-Ubuntu SMP Fri Apr 26 12:29:36 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Eclipse Adoptium-11.0.17+8 | | checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/artifact/yetus-general-check/output/diff-checkstyle-hbase-rest.txt | | spotless | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/artifact/yetus-general-check/output/patch-spotless.txt | | Max. process+thread count | 81 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/3/console | | versions | git=2.34.1 maven=3.8.6 spotbugs=4.7.3 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132777175 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 27s | Docker mode activated. | | -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 3m 15s | master passed | | +1 :green_heart: | compile | 0m 18s | master passed | | +1 :green_heart: | shadedjars | 5m 37s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 14s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 57s | the patch passed | | +1 :green_heart: | compile | 0m 17s | the patch passed | | +1 :green_heart: | javac | 0m 17s | the patch passed | | +1 :green_heart: | shadedjars | 5m 41s | patch has no errors when building our shaded downstream artifacts. | | -0 :warning: | javadoc | 0m 13s | hbase-rest generated 3 new + 0 unchanged - 0 fixed = 3 total (was 0) | ||| _ Other Tests _ | | +1 :green_heart: | unit | 3m 29s | hbase-rest in the patch passed. | | | | 23m 18s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux 6799c0252586 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Eclipse Adoptium-17.0.10+7 | | javadoc | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/artifact/yetus-jdk17-hadoop3-check/output/diff-javadoc-javadoc-hbase-rest.txt | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/testReport/ | | Max. process+thread count | 1753 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132776835 :broken_heart: **-1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 3m 32s | Docker mode activated. | ||| _ Prechecks _ | | +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. | | +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. | | +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 58s | master passed | | +1 :green_heart: | compile | 0m 31s | master passed | | +1 :green_heart: | checkstyle | 0m 12s | master passed | | +1 :green_heart: | spotless | 0m 45s | branch has no errors when running spotless:check. | | +1 :green_heart: | spotbugs | 0m 30s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 48s | the patch passed | | +1 :green_heart: | compile | 0m 27s | the patch passed | | +1 :green_heart: | javac | 0m 27s | the patch passed | | -0 :warning: | checkstyle | 0m 12s | hbase-rest: The patch generated 1 new + 23 unchanged - 0 fixed = 24 total (was 23) | | +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. | | +1 :green_heart: | hadoopcheck | 4m 55s | Patch does not cause any errors with Hadoop 3.3.6. | | -1 :x: | spotless | 0m 40s | patch has 24 errors when running spotless:check, run spotless:apply to fix. | | +1 :green_heart: | spotbugs | 0m 37s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | asflicense | 0m 12s | The patch does not generate ASF License warnings. | | | | 24m 18s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/artifact/yetus-general-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile | | uname | Linux 44639d493846 5.4.0-174-generic #193-Ubuntu SMP Thu Mar 7 14:29:28 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Eclipse Adoptium-11.0.17+8 | | checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/artifact/yetus-general-check/output/diff-checkstyle-hbase-rest.txt | | spotless | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/artifact/yetus-general-check/output/patch-spotless.txt | | Max. process+thread count | 79 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/console | | versions | git=2.34.1 maven=3.8.6 spotbugs=4.7.3 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132777107 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 3m 28s | Docker mode activated. | | -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 37s | master passed | | +1 :green_heart: | compile | 0m 17s | master passed | | +1 :green_heart: | shadedjars | 5m 9s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 16s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 28s | the patch passed | | +1 :green_heart: | compile | 0m 18s | the patch passed | | +1 :green_heart: | javac | 0m 18s | the patch passed | | +1 :green_heart: | shadedjars | 5m 6s | patch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 14s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | unit | 3m 24s | hbase-rest in the patch passed. | | | | 24m 10s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux 7219471599b3 5.4.0-182-generic #202-Ubuntu SMP Fri Apr 26 12:29:36 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Temurin-1.8.0_352-b08 | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/testReport/ | | Max. process+thread count | 1579 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2132773783 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 13s | Docker mode activated. | | -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 3m 0s | master passed | | +1 :green_heart: | compile | 0m 19s | master passed | | +1 :green_heart: | shadedjars | 5m 15s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 16s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 48s | the patch passed | | +1 :green_heart: | compile | 0m 18s | the patch passed | | +1 :green_heart: | javac | 0m 18s | the patch passed | | +1 :green_heart: | shadedjars | 5m 15s | patch has no errors when building our shaded downstream artifacts. | | -0 :warning: | javadoc | 0m 15s | hbase-rest generated 3 new + 0 unchanged - 0 fixed = 3 total (was 0) | ||| _ Other Tests _ | | +1 :green_heart: | unit | 3m 42s | hbase-rest in the patch passed. | | | | 22m 16s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux 0ee634fe5cbd 5.4.0-182-generic #202-Ubuntu SMP Fri Apr 26 12:29:36 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / 2130e5410f | | Default Java | Eclipse Adoptium-11.0.17+8 | | javadoc | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/artifact/yetus-jdk11-hadoop3-check/output/diff-javadoc-javadoc-hbase-rest.txt | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/testReport/ | | Max. process+thread count | 1452 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/2/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
stoty commented on code in PR #5943:
URL: https://github.com/apache/hbase/pull/5943#discussion_r1615540093
##
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;
Review Comment:
Thank you, fixed.
##
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();
+ }
+
+ public Message messageFromObject();
Review Comment:
Thank you, fixed.
--
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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
subrat-mishra commented on code in PR #5943:
URL: https://github.com/apache/hbase/pull/5943#discussion_r1615517740
##
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();
+ }
+
+ public Message messageFromObject();
Review Comment:
nit: Modifier 'public' is also redundant for interface members. It can be
removed.
--
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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
subrat-mishra commented on code in PR #5943:
URL: https://github.com/apache/hbase/pull/5943#discussion_r1614452647
##
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;
Review Comment:
nit: static/final is redundant as all the fields in an interface are
implicitly static/final/public.
--
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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2127439225 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 30s | Docker mode activated. | | -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 4m 8s | master passed | | +1 :green_heart: | compile | 0m 26s | master passed | | +1 :green_heart: | shadedjars | 6m 40s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 16s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 3m 40s | the patch passed | | +1 :green_heart: | compile | 0m 21s | the patch passed | | +1 :green_heart: | javac | 0m 21s | the patch passed | | +1 :green_heart: | shadedjars | 6m 42s | patch has no errors when building our shaded downstream artifacts. | | -0 :warning: | javadoc | 0m 17s | hbase-rest generated 3 new + 0 unchanged - 0 fixed = 3 total (was 0) | ||| _ Other Tests _ | | +1 :green_heart: | unit | 5m 3s | hbase-rest in the patch passed. | | | | 29m 12s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux 25bdf3f9912e 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / e5b581686d | | Default Java | Eclipse Adoptium-11.0.17+8 | | javadoc | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/artifact/yetus-jdk11-hadoop3-check/output/diff-javadoc-javadoc-hbase-rest.txt | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/testReport/ | | Max. process+thread count | 1667 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2127433769 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 46s | Docker mode activated. | | -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 3m 9s | master passed | | +1 :green_heart: | compile | 0m 20s | master passed | | +1 :green_heart: | shadedjars | 6m 11s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 15s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 3m 25s | the patch passed | | +1 :green_heart: | compile | 0m 23s | the patch passed | | +1 :green_heart: | javac | 0m 23s | the patch passed | | +1 :green_heart: | shadedjars | 6m 9s | patch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 14s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | unit | 4m 19s | hbase-rest in the patch passed. | | | | 26m 18s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux 0994b4069d7d 5.4.0-174-generic #193-Ubuntu SMP Thu Mar 7 14:29:28 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / e5b581686d | | Default Java | Temurin-1.8.0_352-b08 | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/testReport/ | | Max. process+thread count | 1519 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2127429785 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 29s | Docker mode activated. | | -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck | ||| _ Prechecks _ | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 3m 24s | master passed | | +1 :green_heart: | compile | 0m 22s | master passed | | +1 :green_heart: | shadedjars | 5m 55s | branch has no errors when building our shaded downstream artifacts. | | +1 :green_heart: | javadoc | 0m 15s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 3m 0s | the patch passed | | +1 :green_heart: | compile | 0m 18s | the patch passed | | +1 :green_heart: | javac | 0m 18s | the patch passed | | +1 :green_heart: | shadedjars | 5m 51s | patch has no errors when building our shaded downstream artifacts. | | -0 :warning: | javadoc | 0m 13s | hbase-rest generated 3 new + 0 unchanged - 0 fixed = 3 total (was 0) | ||| _ Other Tests _ | | +1 :green_heart: | unit | 3m 44s | hbase-rest in the patch passed. | | | | 24m 38s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | javac javadoc unit shadedjars compile | | uname | Linux 350249c0a0d3 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / e5b581686d | | Default Java | Eclipse Adoptium-17.0.10+7 | | javadoc | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/artifact/yetus-jdk17-hadoop3-check/output/diff-javadoc-javadoc-hbase-rest.txt | | Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/testReport/ | | Max. process+thread count | 1698 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/console | | versions | git=2.34.1 maven=3.8.6 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
Re: [PR] HBASE-28613 Use streaming when marshalling protobuf REST output [hbase]
Apache-HBase commented on PR #5943: URL: https://github.com/apache/hbase/pull/5943#issuecomment-2127423289 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Comment | |::|--:|:|:| | +0 :ok: | reexec | 0m 28s | Docker mode activated. | ||| _ Prechecks _ | | +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. | | +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. | | +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. | ||| _ master Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 57s | master passed | | +1 :green_heart: | compile | 0m 26s | master passed | | +1 :green_heart: | checkstyle | 0m 10s | master passed | | +1 :green_heart: | spotless | 0m 42s | branch has no errors when running spotless:check. | | +1 :green_heart: | spotbugs | 0m 28s | master passed | ||| _ Patch Compile Tests _ | | +1 :green_heart: | mvninstall | 2m 44s | the patch passed | | +1 :green_heart: | compile | 0m 25s | the patch passed | | +1 :green_heart: | javac | 0m 25s | the patch passed | | +1 :green_heart: | checkstyle | 0m 9s | the patch passed | | +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. | | +1 :green_heart: | hadoopcheck | 5m 12s | Patch does not cause any errors with Hadoop 3.3.6. | | +1 :green_heart: | spotless | 0m 40s | patch has no errors when running spotless:check. | | +1 :green_heart: | spotbugs | 0m 31s | the patch passed | ||| _ Other Tests _ | | +1 :green_heart: | asflicense | 0m 10s | The patch does not generate ASF License warnings. | | | | 21m 27s | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/artifact/yetus-general-check/output/Dockerfile | | GITHUB PR | https://github.com/apache/hbase/pull/5943 | | Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile | | uname | Linux 6fe40d69f8f2 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/hbase-personality.sh | | git revision | master / e5b581686d | | Default Java | Eclipse Adoptium-11.0.17+8 | | Max. process+thread count | 79 (vs. ulimit of 3) | | modules | C: hbase-rest U: hbase-rest | | Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5943/1/console | | versions | git=2.34.1 maven=3.8.6 spotbugs=4.7.3 | | Powered by | Apache Yetus 0.12.0 https://yetus.apache.org | This message was automatically generated. -- 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
