You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "bbeaudreault (via GitHub)" <gi...@apache.org> on 2024/02/22 01:05:01 UTC
[PR] HBASE-28390 WAL value compression fails for cells with large values [hbase]
bbeaudreault opened a new pull request, #5696:
URL: https://github.com/apache/hbase/pull/5696
Unify all of the WAL compression tests to utilize tests defined in the base class. Add a `testLarge` test which writes large buffers. Prior to the fixes CompressionContext and WALDecompressionBoundedDelegatingInputStream, these tests all failed.
--
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
Re: [PR] HBASE-28390 WAL value compression fails for cells with large values [hbase]
Posted by "bbeaudreault (via GitHub)" <gi...@apache.org>.
bbeaudreault commented on code in PR #5696:
URL: https://github.com/apache/hbase/pull/5696#discussion_r1500054676
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/CompressionContext.java:
##########
@@ -129,9 +130,25 @@ public void decompress(InputStream in, int inLength, byte[] outArray, int outOff
} else {
lowerIn.reset(in, inLength);
IOUtils.readFully(compressedIn, outArray, outOffset, outLength);
+ // if the uncompressed size was larger than the configured buffer size for the codec,
+ // the BlockCompressorStream will have left an extra 4 bytes hanging. This represents a size
+ // for the next segment, and it should be 0. See HBASE-28390
+ if (lowerIn.available() == 4) {
+ int remaining = rawReadInt(lowerIn);
+ assert remaining == 0;
+ }
}
}
+ private int rawReadInt(InputStream in) throws IOException {
Review Comment:
I tried looking and couldn't find one. I'll try looking again
--
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
Re: [PR] HBASE-28390 WAL value compression fails for cells with large values [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #5696:
URL: https://github.com/apache/hbase/pull/5696#issuecomment-1958754792
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 38s | 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 _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 5s | master passed |
| +1 :green_heart: | compile | 2m 22s | master passed |
| +1 :green_heart: | shadedjars | 5m 14s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 44s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 45s | the patch passed |
| +1 :green_heart: | compile | 2m 21s | the patch passed |
| +1 :green_heart: | javac | 2m 21s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 11s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 43s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 230m 8s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 1m 16s | hbase-compression-aircompressor in the patch passed. |
| +1 :green_heart: | unit | 0m 57s | hbase-compression-brotli in the patch passed. |
| +1 :green_heart: | unit | 0m 47s | hbase-compression-lz4 in the patch passed. |
| +1 :green_heart: | unit | 0m 48s | hbase-compression-snappy in the patch passed. |
| +1 :green_heart: | unit | 2m 53s | hbase-compression-xz in the patch passed. |
| +1 :green_heart: | unit | 6m 15s | hbase-compression-zstd in the patch passed. |
| | | 274m 1s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.44 ServerAPI=1.44 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5696/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/5696 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 2d5cc2c686a7 5.4.0-169-generic #187-Ubuntu SMP Thu Nov 23 14:52:28 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 7be588e0d4 |
| Default Java | Eclipse Adoptium-11.0.17+8 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5696/1/testReport/ |
| Max. process+thread count | 5630 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-compression/hbase-compression-aircompressor hbase-compression/hbase-compression-brotli hbase-compression/hbase-compression-lz4 hbase-compression/hbase-compression-snappy hbase-compression/hbase-compression-xz hbase-compression/hbase-compression-zstd U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5696/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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28390 WAL value compression fails for cells with large values [hbase]
Posted by "bbeaudreault (via GitHub)" <gi...@apache.org>.
bbeaudreault commented on code in PR #5696:
URL: https://github.com/apache/hbase/pull/5696#discussion_r1500854815
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/CompressionContext.java:
##########
@@ -129,9 +130,25 @@ public void decompress(InputStream in, int inLength, byte[] outArray, int outOff
} else {
lowerIn.reset(in, inLength);
IOUtils.readFully(compressedIn, outArray, outOffset, outLength);
+ // if the uncompressed size was larger than the configured buffer size for the codec,
+ // the BlockCompressorStream will have left an extra 4 bytes hanging. This represents a size
+ // for the next segment, and it should be 0. See HBASE-28390
+ if (lowerIn.available() == 4) {
+ int remaining = rawReadInt(lowerIn);
+ assert remaining == 0;
+ }
}
}
+ private int rawReadInt(InputStream in) throws IOException {
Review Comment:
Actually, there is no IOUtils.readInt in commons-io. I realized that the IOUtils I was referring to (while on mobile) was a different library that we don't have a dependency on.
We have a variety of methods for reading ints from byte[] or ByteBuffer, but not for InputStream that I can find. I could read the bytes into a byte[4] and use one of the utils, but I'd rather not allocate an unnecessary byte array. I'm going to leave this alone for now.
--
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
Re: [PR] HBASE-28390 WAL value compression fails for cells with large values [hbase]
Posted by "bbeaudreault (via GitHub)" <gi...@apache.org>.
bbeaudreault commented on code in PR #5696:
URL: https://github.com/apache/hbase/pull/5696#discussion_r1500055293
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/CompressionContext.java:
##########
@@ -129,9 +130,25 @@ public void decompress(InputStream in, int inLength, byte[] outArray, int outOff
} else {
lowerIn.reset(in, inLength);
IOUtils.readFully(compressedIn, outArray, outOffset, outLength);
+ // if the uncompressed size was larger than the configured buffer size for the codec,
+ // the BlockCompressorStream will have left an extra 4 bytes hanging. This represents a size
+ // for the next segment, and it should be 0. See HBASE-28390
+ if (lowerIn.available() == 4) {
+ int remaining = rawReadInt(lowerIn);
+ assert remaining == 0;
+ }
}
}
+ private int rawReadInt(InputStream in) throws IOException {
Review Comment:
Woops I think IOUtils.readInt should work. Any other comments/concerns other than that?
--
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
Re: [PR] HBASE-28390 WAL value compression fails for cells with large values [hbase]
Posted by "apurtell (via GitHub)" <gi...@apache.org>.
apurtell commented on code in PR #5696:
URL: https://github.com/apache/hbase/pull/5696#discussion_r1500056526
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/CompressionContext.java:
##########
@@ -129,9 +130,25 @@ public void decompress(InputStream in, int inLength, byte[] outArray, int outOff
} else {
lowerIn.reset(in, inLength);
IOUtils.readFully(compressedIn, outArray, outOffset, outLength);
+ // if the uncompressed size was larger than the configured buffer size for the codec,
+ // the BlockCompressorStream will have left an extra 4 bytes hanging. This represents a size
+ // for the next segment, and it should be 0. See HBASE-28390
+ if (lowerIn.available() == 4) {
+ int remaining = rawReadInt(lowerIn);
+ assert remaining == 0;
+ }
}
}
+ private int rawReadInt(InputStream in) throws IOException {
Review Comment:
Left an approving review with the rest. Thanks!
--
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
Re: [PR] HBASE-28390 WAL value compression fails for cells with large values [hbase]
Posted by "apurtell (via GitHub)" <gi...@apache.org>.
apurtell commented on code in PR #5696:
URL: https://github.com/apache/hbase/pull/5696#discussion_r1500052501
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/CompressionContext.java:
##########
@@ -129,9 +130,25 @@ public void decompress(InputStream in, int inLength, byte[] outArray, int outOff
} else {
lowerIn.reset(in, inLength);
IOUtils.readFully(compressedIn, outArray, outOffset, outLength);
+ // if the uncompressed size was larger than the configured buffer size for the codec,
+ // the BlockCompressorStream will have left an extra 4 bytes hanging. This represents a size
+ // for the next segment, and it should be 0. See HBASE-28390
+ if (lowerIn.available() == 4) {
+ int remaining = rawReadInt(lowerIn);
+ assert remaining == 0;
+ }
}
}
+ private int rawReadInt(InputStream in) throws IOException {
Review Comment:
I'm surprised commons-io or our own util code doesn't have something like this already.
--
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
Re: [PR] HBASE-28390 WAL value compression fails for cells with large values [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #5696:
URL: https://github.com/apache/hbase/pull/5696#issuecomment-1958526618
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 15s | 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 _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 6s | master passed |
| +1 :green_heart: | compile | 4m 37s | master passed |
| +1 :green_heart: | checkstyle | 1m 27s | master passed |
| +1 :green_heart: | spotless | 0m 48s | branch has no errors when running spotless:check. |
| +1 :green_heart: | spotbugs | 3m 34s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 48s | the patch passed |
| +1 :green_heart: | compile | 4m 12s | the patch passed |
| +1 :green_heart: | javac | 4m 12s | the patch passed |
| -0 :warning: | checkstyle | 0m 35s | hbase-server: The patch generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 4m 54s | 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 | 4m 26s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 1m 7s | The patch does not generate ASF License warnings. |
| | | 42m 30s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.44 ServerAPI=1.44 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5696/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/5696 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti spotless checkstyle compile |
| uname | Linux c4438ae23f31 5.4.0-166-generic #183-Ubuntu SMP Mon Oct 2 11:28:33 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 7be588e0d4 |
| Default Java | Eclipse Adoptium-11.0.17+8 |
| checkstyle | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5696/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| Max. process+thread count | 79 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-compression/hbase-compression-aircompressor hbase-compression/hbase-compression-brotli hbase-compression/hbase-compression-lz4 hbase-compression/hbase-compression-snappy hbase-compression/hbase-compression-xz hbase-compression/hbase-compression-zstd U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5696/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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28390 WAL value compression fails for cells with large values [hbase]
Posted by "bbeaudreault (via GitHub)" <gi...@apache.org>.
bbeaudreault merged PR #5696:
URL: https://github.com/apache/hbase/pull/5696
--
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
Re: [PR] HBASE-28390 WAL value compression fails for cells with large values [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #5696:
URL: https://github.com/apache/hbase/pull/5696#issuecomment-1958754838
: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 _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 2m 52s | master passed |
| +1 :green_heart: | compile | 1m 56s | master passed |
| +1 :green_heart: | shadedjars | 5m 45s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 24s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 2m 31s | the patch passed |
| +1 :green_heart: | compile | 1m 56s | the patch passed |
| +1 :green_heart: | javac | 1m 56s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 41s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 22s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 232m 39s | hbase-server in the patch passed. |
| +1 :green_heart: | unit | 1m 9s | hbase-compression-aircompressor in the patch passed. |
| +1 :green_heart: | unit | 0m 51s | hbase-compression-brotli in the patch passed. |
| +1 :green_heart: | unit | 0m 41s | hbase-compression-lz4 in the patch passed. |
| +1 :green_heart: | unit | 0m 42s | hbase-compression-snappy in the patch passed. |
| +1 :green_heart: | unit | 3m 19s | hbase-compression-xz in the patch passed. |
| +1 :green_heart: | unit | 5m 22s | hbase-compression-zstd in the patch passed. |
| | | 273m 48s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5696/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/5696 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux f180f4d304a8 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 / 7be588e0d4 |
| Default Java | Temurin-1.8.0_352-b08 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5696/1/testReport/ |
| Max. process+thread count | 5104 (vs. ulimit of 30000) |
| modules | C: hbase-server hbase-compression/hbase-compression-aircompressor hbase-compression/hbase-compression-brotli hbase-compression/hbase-compression-lz4 hbase-compression/hbase-compression-snappy hbase-compression/hbase-compression-xz hbase-compression/hbase-compression-zstd U: . |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-5696/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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-28390 WAL value compression fails for cells with large values [hbase]
Posted by "apurtell (via GitHub)" <gi...@apache.org>.
apurtell commented on code in PR #5696:
URL: https://github.com/apache/hbase/pull/5696#discussion_r1500056258
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/CompressionContext.java:
##########
@@ -129,9 +130,25 @@ public void decompress(InputStream in, int inLength, byte[] outArray, int outOff
} else {
lowerIn.reset(in, inLength);
IOUtils.readFully(compressedIn, outArray, outOffset, outLength);
+ // if the uncompressed size was larger than the configured buffer size for the codec,
+ // the BlockCompressorStream will have left an extra 4 bytes hanging. This represents a size
+ // for the next segment, and it should be 0. See HBASE-28390
+ if (lowerIn.available() == 4) {
Review Comment:
This part is ugly but I don't have an alternative suggestion. At least we know both in the WAL read and HFile read cases we won't be contending with a short read, so available is going to be either 0 or 4 and otherwise we'd have had an EOFException thrown.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/wal/CompressedWALTestBase.java:
##########
@@ -67,14 +71,36 @@ public class CompressedWALTestBase {
Arrays.fill(VALUE, off, (off += 1597), (byte) 'Q');
}
- public void doTest(TableName tableName) throws Exception {
+ @Test
Review Comment:
Thanks for cleaning this up and removing all the unneeded boilerplate.
--
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