You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2022/07/11 22:14:48 UTC
[GitHub] [hbase] apurtell opened a new pull request, #4613: HBASE-27097 SimpleRpcServer is broken
apurtell opened a new pull request, #4613:
URL: https://github.com/apache/hbase/pull/4613
Replace `BufferChain#write(channel,int)` with a simpler `#write(channel)` implementation that does not attempt to "chunk" data to be written. This method was used exclusively by `SimpleRpcServer`. The code was unnecessarily complex and caused short writes when values were large, which were exposed by MAC verification failures when SASL is enabled, so was corrected and simplified. Any difference in performance from this change will be limited to `SimpleRpcServer`. Testing under load confirms the fix and does not show significant regression.
`SimpleRpcServer` and its related code is now also marked as `@Deprecated`.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4613: HBASE-27097 SimpleRpcServer is broken
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4613:
URL: https://github.com/apache/hbase/pull/4613#issuecomment-1180997941
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 38s | 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 33s | master passed |
| +1 :green_heart: | compile | 2m 12s | master passed |
| +1 :green_heart: | checkstyle | 0m 30s | master passed |
| +1 :green_heart: | spotless | 0m 41s | branch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 1m 14s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 10s | the patch passed |
| +1 :green_heart: | compile | 2m 11s | the patch passed |
| +1 :green_heart: | javac | 2m 11s | the patch passed |
| -0 :warning: | checkstyle | 0m 28s | hbase-server: The patch generated 4 new + 6 unchanged - 1 fixed = 10 total (was 7) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 11m 57s | Patch does not cause any errors with Hadoop 3.1.2 3.2.2 3.3.1. |
| +1 :green_heart: | spotless | 0m 40s | patch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 1m 18s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 9s | The patch does not generate ASF License warnings. |
| | | 32m 13s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4613/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4613 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile |
| uname | Linux d1e0fb7f1319 5.4.0-1071-aws #76~18.04.1-Ubuntu SMP Mon Mar 28 17:49:57 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / f3545cd73a |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4613/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 64 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4613/1/console |
| versions | git=2.17.1 maven=3.6.3 spotbugs=4.2.2 |
| 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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4613: HBASE-27097 SimpleRpcServer is broken
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4613:
URL: https://github.com/apache/hbase/pull/4613#issuecomment-1181222631
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 41s | 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 46s | master passed |
| +1 :green_heart: | compile | 0m 42s | master passed |
| +1 :green_heart: | shadedjars | 3m 55s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 25s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 31s | the patch passed |
| +1 :green_heart: | compile | 0m 40s | the patch passed |
| +1 :green_heart: | javac | 0m 40s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 49s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 22s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 193m 17s | hbase-server in the patch passed. |
| | | 211m 8s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4613/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4613 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 68434fdadb28 5.4.0-1071-aws #76~18.04.1-Ubuntu SMP Mon Mar 28 17:49:57 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / f3545cd73a |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4613/1/testReport/ |
| Max. process+thread count | 2585 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4613/1/console |
| versions | git=2.17.1 maven=3.6.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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on a diff in pull request #4613: HBASE-27097 SimpleRpcServer is broken
Posted by GitBox <gi...@apache.org>.
apurtell commented on code in PR #4613:
URL: https://github.com/apache/hbase/pull/4613#discussion_r918457818
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java:
##########
@@ -61,51 +60,27 @@ boolean hasRemaining() {
return remaining > 0;
}
- /**
- * Write out our chain of buffers in chunks
- * @param channel Where to write
- * @param chunkSize Size of chunks to write.
- * @return Amount written. n
- */
- long write(GatheringByteChannel channel, int chunkSize) throws IOException {
- int chunkRemaining = chunkSize;
- ByteBuffer lastBuffer = null;
- int bufCount = 0;
- int restoreLimit = -1;
-
- while (chunkRemaining > 0 && bufferOffset + bufCount < buffers.length) {
- lastBuffer = buffers[bufferOffset + bufCount];
- if (!lastBuffer.hasRemaining()) {
- bufferOffset++;
- continue;
- }
- bufCount++;
- if (lastBuffer.remaining() > chunkRemaining) {
- restoreLimit = lastBuffer.limit();
- lastBuffer.limit(lastBuffer.position() + chunkRemaining);
- chunkRemaining = 0;
- break;
- } else {
- chunkRemaining -= lastBuffer.remaining();
- }
- }
- assert lastBuffer != null;
- if (chunkRemaining == chunkSize) {
- assert !hasRemaining();
- // no data left to write
+ long write(GatheringByteChannel channel) throws IOException {
+ if (!hasRemaining()) {
return 0;
}
- try {
- long ret = channel.write(buffers, bufferOffset, bufCount);
- if (ret > 0) {
- remaining = (int) (remaining - ret);
- }
- return ret;
- } finally {
- if (restoreLimit >= 0) {
- lastBuffer.limit(restoreLimit);
+ long written = 0;
+ for (ByteBuffer bb : this.buffers) {
+ if (bb.hasRemaining()) {
+ final int pos = bb.position();
+ final int result = channel.write(bb);
+ if (result <= 0) {
+ // Write error. Return how much we were able to write until now.
+ return written;
+ }
+ // Adjust the position of buffers already written so we don't write out
+ // duplicate data upon retry of incomplete write with the same buffer chain.
+ bb.position(pos + result);
Review Comment:
I don't think we will hit this case. If the write for a call response fails an exception will be raised and that's it for that particular call and response. If the client decides to retry there will be a new call, and a new response, and a new write with a new bufferchain. That said I wanted this code to be correct if it happens, perhaps after some future change.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #4613: HBASE-27097 SimpleRpcServer is broken
Posted by GitBox <gi...@apache.org>.
apurtell commented on PR #4613:
URL: https://github.com/apache/hbase/pull/4613#issuecomment-1181198055
We have some noise in recent precommit results. Let me run the suite locally too to check this one.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on a diff in pull request #4613: HBASE-27097 SimpleRpcServer is broken
Posted by GitBox <gi...@apache.org>.
apurtell commented on code in PR #4613:
URL: https://github.com/apache/hbase/pull/4613#discussion_r918456929
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/SimpleRpcServer.java:
##########
@@ -69,6 +69,7 @@
* put itself on new queue for Responder to pull from and return result to client.
* @see BlockingRpcClient
*/
+@Deprecated()
Review Comment:
Sure, will do that on commit.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #4613: HBASE-27097 SimpleRpcServer is broken
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on PR #4613:
URL: https://github.com/apache/hbase/pull/4613#issuecomment-1181236677
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 10s | Docker mode activated. |
| -0 :warning: | yetus | 0m 4s | 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 41s | master passed |
| +1 :green_heart: | shadedjars | 3m 52s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 27s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 2m 22s | the patch passed |
| +1 :green_heart: | compile | 0m 42s | the patch passed |
| +1 :green_heart: | javac | 0m 42s | the patch passed |
| +1 :green_heart: | shadedjars | 3m 51s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 24s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 219m 44s | hbase-server in the patch failed. |
| | | 237m 13s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4613/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/4613 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux e99c6227cb39 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / f3545cd73a |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| unit | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4613/1/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4613/1/testReport/ |
| Max. process+thread count | 2686 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-4613/1/console |
| versions | git=2.17.1 maven=3.6.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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell merged pull request #4613: HBASE-27097 SimpleRpcServer is broken
Posted by GitBox <gi...@apache.org>.
apurtell merged PR #4613:
URL: https://github.com/apache/hbase/pull/4613
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a diff in pull request #4613: HBASE-27097 SimpleRpcServer is broken
Posted by GitBox <gi...@apache.org>.
virajjasani commented on code in PR #4613:
URL: https://github.com/apache/hbase/pull/4613#discussion_r918402298
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/SimpleRpcServer.java:
##########
@@ -69,6 +69,7 @@
* put itself on new queue for Responder to pull from and return result to client.
* @see BlockingRpcClient
*/
+@Deprecated()
Review Comment:
Shall we add a comment stating that this will be likely removed from HBase 3.0 onwards?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/BufferChain.java:
##########
@@ -61,51 +60,27 @@ boolean hasRemaining() {
return remaining > 0;
}
- /**
- * Write out our chain of buffers in chunks
- * @param channel Where to write
- * @param chunkSize Size of chunks to write.
- * @return Amount written. n
- */
- long write(GatheringByteChannel channel, int chunkSize) throws IOException {
- int chunkRemaining = chunkSize;
- ByteBuffer lastBuffer = null;
- int bufCount = 0;
- int restoreLimit = -1;
-
- while (chunkRemaining > 0 && bufferOffset + bufCount < buffers.length) {
- lastBuffer = buffers[bufferOffset + bufCount];
- if (!lastBuffer.hasRemaining()) {
- bufferOffset++;
- continue;
- }
- bufCount++;
- if (lastBuffer.remaining() > chunkRemaining) {
- restoreLimit = lastBuffer.limit();
- lastBuffer.limit(lastBuffer.position() + chunkRemaining);
- chunkRemaining = 0;
- break;
- } else {
- chunkRemaining -= lastBuffer.remaining();
- }
- }
- assert lastBuffer != null;
- if (chunkRemaining == chunkSize) {
- assert !hasRemaining();
- // no data left to write
+ long write(GatheringByteChannel channel) throws IOException {
+ if (!hasRemaining()) {
return 0;
}
- try {
- long ret = channel.write(buffers, bufferOffset, bufCount);
- if (ret > 0) {
- remaining = (int) (remaining - ret);
- }
- return ret;
- } finally {
- if (restoreLimit >= 0) {
- lastBuffer.limit(restoreLimit);
+ long written = 0;
+ for (ByteBuffer bb : this.buffers) {
+ if (bb.hasRemaining()) {
+ final int pos = bb.position();
+ final int result = channel.write(bb);
+ if (result <= 0) {
+ // Write error. Return how much we were able to write until now.
+ return written;
+ }
+ // Adjust the position of buffers already written so we don't write out
+ // duplicate data upon retry of incomplete write with the same buffer chain.
+ bb.position(pos + result);
Review Comment:
Ah, I thought this might have been inferred in one of the functions here, but looks like this was a miss anyways.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org