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 2021/06/10 19:53:39 UTC
[GitHub] [hbase] apurtell opened a new pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
apurtell opened a new pull request #3377:
URL: https://github.com/apache/hbase/pull/3377
Depending on which compression codec is used, a short read of the compressed bytes can cause catastrophic errors that confuse the WAL reader. This problem can manifest when the reader is actively tailing the WAL for replication. The input stream's available() method sometimes lies so cannot be relied upon. To avoid these issues when WAL value compression is enabled ensure all bytes of the compressed value are read in and thus available before submitting the payload to the decompressor.
Adds new unit tests TestReplicationCompressedWAL and TestReplicationValueCompressedWAL.
Without the WALCellCodec change TestReplicationValueCompressedWAL will fail.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
apurtell commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-860265564
@bharathv
BoundedDelegatingInputStream was trying too hard in it's available() method.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-860927887
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 12s | 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 21s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 59s | master passed |
| +1 :green_heart: | compile | 1m 48s | master passed |
| +1 :green_heart: | shadedjars | 9m 9s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 9s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 49s | the patch passed |
| +1 :green_heart: | compile | 2m 6s | the patch passed |
| +1 :green_heart: | javac | 2m 6s | the patch passed |
| +1 :green_heart: | shadedjars | 9m 21s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 8s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 2m 18s | hbase-common in the patch passed. |
| -1 :x: | unit | 227m 16s | hbase-server in the patch failed. |
| | | 268m 4s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/3/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3377 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux f08cb3ba9cb0 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / ba6995e083 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/3/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/3/testReport/ |
| Max. process+thread count | 2644 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/3/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell edited a comment on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
apurtell edited a comment on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-858994276
@bharathv Had to undo that read side optimization we discussed on #3244. It's fine for readers that operate on closed and completed WAL files. We were missing coverage of the other case, when the WAL is actively tailed. Added that coverage. Determined the optimization is not a good idea for that case via unit test failure (and confirmed fix).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-861138878
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 43s | 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 _ |
| +0 :ok: | mvndep | 0m 27s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 6m 37s | master passed |
| +1 :green_heart: | compile | 2m 17s | master passed |
| +1 :green_heart: | shadedjars | 11m 21s | 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 18s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 51s | the patch passed |
| +1 :green_heart: | compile | 2m 9s | the patch passed |
| +1 :green_heart: | javac | 2m 9s | the patch passed |
| +1 :green_heart: | shadedjars | 11m 1s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 17s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 2m 29s | hbase-common in the patch passed. |
| -1 :x: | unit | 147m 9s | hbase-server in the patch failed. |
| | | 195m 50s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/4/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3377 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 9c3237f23193 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / a35ec994b9 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| unit | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/4/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-hbase-server.txt |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/4/testReport/ |
| Max. process+thread count | 3918 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/4/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
apurtell commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-859055987
Checkstyle nit already addressed (c2a3a90). Whitespace nit noted, will fix if there is a round of review or at commit time.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-861154858
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 59s | 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 20s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 15s | master passed |
| +1 :green_heart: | compile | 1m 25s | master passed |
| +1 :green_heart: | shadedjars | 8m 58s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 0s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 2s | the patch passed |
| +1 :green_heart: | compile | 1m 26s | the patch passed |
| +1 :green_heart: | javac | 1m 26s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 59s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 56s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 53s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 212m 30s | hbase-server in the patch passed. |
| | | 249m 1s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/4/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3377 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 7770bdf1ec17 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / a35ec994b9 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/4/testReport/ |
| Max. process+thread count | 3318 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/4/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell edited a comment on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
apurtell edited a comment on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-858994276
@bharathv Had to undo that read side optimization we discussed on #3244. It's fine for readers that operate on closed and completed WAL files. We were missing coverage of the other case, when the WAL is actively tailed. Added that coverage. Determined the optimization is not a good idea for that case.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-859161135
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-859138246
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 5s | 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 55s | master passed |
| +1 :green_heart: | compile | 1m 1s | master passed |
| +1 :green_heart: | shadedjars | 8m 14s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 38s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 44s | the patch passed |
| +1 :green_heart: | compile | 1m 0s | the patch passed |
| +1 :green_heart: | javac | 1m 0s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 12s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 37s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 151m 45s | hbase-server in the patch passed. |
| | | 182m 31s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/1/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3377 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux c84a74b15cab 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 6b81ff94a5 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/1/testReport/ |
| Max. process+thread count | 4157 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-860784409
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 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 _ |
| +0 :ok: | mvndep | 0m 32s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 45s | master passed |
| +1 :green_heart: | compile | 4m 1s | master passed |
| +1 :green_heart: | checkstyle | 1m 29s | master passed |
| +1 :green_heart: | spotbugs | 2m 48s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 42s | the patch passed |
| +1 :green_heart: | compile | 4m 0s | the patch passed |
| +1 :green_heart: | javac | 4m 0s | the patch passed |
| +1 :green_heart: | checkstyle | 1m 20s | the patch passed |
| -0 :warning: | whitespace | 0m 0s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
| +1 :green_heart: | hadoopcheck | 18m 35s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| +1 :green_heart: | spotbugs | 3m 10s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 25s | The patch does not generate ASF License warnings. |
| | | 54m 56s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3377 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 54ff5f291c38 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / ba6995e083 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| whitespace | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/3/artifact/yetus-general-check/output/whitespace-eol.txt |
| Max. process+thread count | 96 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/3/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-859132011
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 29s | 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 | 4m 34s | master passed |
| +1 :green_heart: | compile | 1m 15s | master passed |
| +1 :green_heart: | shadedjars | 8m 15s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 41s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 15s | the patch passed |
| +1 :green_heart: | compile | 1m 13s | the patch passed |
| +1 :green_heart: | javac | 1m 13s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 14s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 40s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 137m 49s | hbase-server in the patch passed. |
| | | 169m 46s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3377 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 9b7e823c972c 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 6b81ff94a5 |
| Default Java | AdoptOpenJDK-11.0.10+9 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/1/testReport/ |
| Max. process+thread count | 4587 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-859054066
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 17s | 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 | 4m 32s | master passed |
| +1 :green_heart: | compile | 3m 28s | master passed |
| +1 :green_heart: | checkstyle | 1m 13s | master passed |
| +1 :green_heart: | spotbugs | 2m 18s | master passed |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 7s | the patch passed |
| +1 :green_heart: | compile | 3m 30s | the patch passed |
| +1 :green_heart: | javac | 3m 30s | the patch passed |
| -0 :warning: | checkstyle | 1m 12s | hbase-server: The patch generated 1 new + 5 unchanged - 0 fixed = 6 total (was 5) |
| -0 :warning: | whitespace | 0m 0s | The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
| +1 :green_heart: | hadoopcheck | 21m 8s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| +1 :green_heart: | spotbugs | 2m 31s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 14s | The patch does not generate ASF License warnings. |
| | | 55m 18s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3377 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 67912efbd034 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 6b81ff94a5 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/1/artifact/yetus-general-check/output/diff-checkstyle-hbase-server.txt |
| whitespace | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/1/artifact/yetus-general-check/output/whitespace-eol.txt |
| Max. process+thread count | 85 (vs. ulimit of 30000) |
| modules | C: hbase-server U: hbase-server |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
apurtell commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-858994276
@bharathv Had to undo that read side optimization we discussed. It's fine for readers that operate on closed and completed WAL files. We were missing coverage of the other case, when the WAL is actively tailed. Added that coverage. Determined the optimization is not a good idea for that case.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-861093207
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 6s | 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 23s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 13s | master passed |
| +1 :green_heart: | compile | 4m 9s | master passed |
| +1 :green_heart: | checkstyle | 1m 36s | master passed |
| +1 :green_heart: | spotbugs | 2m 54s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 1s | the patch passed |
| +1 :green_heart: | compile | 4m 8s | the patch passed |
| +1 :green_heart: | javac | 4m 8s | the patch passed |
| +1 :green_heart: | checkstyle | 1m 33s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | hadoopcheck | 20m 3s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1 3.3.0. |
| +1 :green_heart: | spotbugs | 3m 14s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 22s | The patch does not generate ASF License warnings. |
| | | 56m 29s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/4/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3377 |
| Optional Tests | dupname asflicense javac spotbugs hadoopcheck hbaseanti checkstyle compile |
| uname | Linux 3dd7a682f5fc 4.15.0-136-generic #140-Ubuntu SMP Thu Jan 28 05:20:47 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / a35ec994b9 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Max. process+thread count | 86 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/4/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bharathv commented on a change in pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
bharathv commented on a change in pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#discussion_r650309749
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
Trying to understand how this actually works, are we relying on the EOFException thrown by readFully here so that upper layers in ProtofbufReader#next() handles it?
If so, curious if the actual fix should be somewhere around ProtobufLogReader#extractHiddenEof()? I mean this works but if we extract the right EOF exception, we can avoid this copy?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
Thanks for the detailed comment.
> If we do not read in the complete segment of the compressed stream, the decompressor, depending on type, will throw random exceptions, maybe IO exceptions, maybe others. These are not EOFExceptions. They permanently confuse the log reader.
Ya, thought so. I think we have to live with that assumption since anyone can plugin any random codec implementation.
> we still need to rewind both the reader and the output stream.
Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
What I had in my mind was an "intercepting" input stream that wraps this compressed input stream and keeps track of bytes read so far. It essentially does what IOUtils.readFully() does but without copying it into a buffer. It just throws EOFException when it is end of stream and readBytesSoFar < totalBytesToBeRead. In some sense we are intercepting the exception that the decompressor runs into even before it happens and we can be sure that it is due to this EOF and not some thing else with the above check. But then I didn't get this part of rewinding the decompressor's internal output stream.
> Flushing the writer is insufficient, although I do that too in this patch to attempt to minimize the time where a tailer might not have a complete WALedit serialization at the current end of file.
Ya, figured that, was wondering if frequent flushing is detrimental to the performance, instead we should probably harden this read path and leave flushing to the upper layers?
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/BoundedDelegatingInputStream.java
##########
@@ -104,8 +104,7 @@ public int available() throws IOException {
if (pos >= limit) {
return 0;
}
- int available = in.available();
- return (int) Math.min(available, limit - pos);
+ return (int) (limit - pos);
Review comment:
Ya, something like this in a wrapping input stream is what I had in my mind but doing this in **BoundedDelegatingInputStream** is even more elegant and clean..
nit: Looks like javadoc needs updation..
I think this behavior is very subtle and not obvious, would be great to back it up with a small comment and how tailing reads depend on 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on a change in pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
apurtell commented on a change in pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#discussion_r650325541
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Trying to understand how this actually works, are we relying on the EOFException thrown by readFully here so that upper layers in ProtofbufReader#next() handles it?
No.
Well, yes, that would happen, but that is not the fix.
There are two problems if we use the input stream directly:
- Sometimes the input stream lies about the number of available bytes
- If we do not read in the complete segment of the compressed stream, the decompressor, depending on type, will throw random exceptions, maybe IO exceptions, maybe others. These are not EOFExceptions. They permanently confuse the log reader.
The fix is to always provide a complete segment of the compressed stream to the decompressor. A buffer and IOUtils.readFully is required for that.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Trying to understand how this actually works, are we relying on the EOFException thrown by readFully here so that upper layers in ProtofbufReader#next() handles it?
No.
Well, yes, that would happen, but that is not the fix.
There are two problems if we use the input stream directly:
- Sometimes the input stream lies about the number of available bytes
- If we do not read in the complete segment of the compressed stream, the decompressor, depending on type, will throw random exceptions, maybe IO exceptions, maybe others. These are not EOFExceptions. They permanently confuse the log reader.
Flushing the writer is insufficient, although I do that too in this patch to attempt to minimize the time where a tailer might not have a complete WALedit serialization at the current end of file.
The fix is to always provide a complete segment of the compressed stream to the decompressor. A buffer and IOUtils.readFully is required for that.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Trying to understand how this actually works, are we relying on the EOFException thrown by readFully here so that upper layers in ProtofbufReader#next() handles it?
No.
Well, yes, that would happen, but that is not the fix.
There are two problems if we use the input stream directly:
- Sometimes the input stream lies about the number of available bytes
- If we do not read in the complete segment of the compressed stream, the decompressor, depending on type, will throw random exceptions, maybe IO exceptions, maybe others. These are not EOFExceptions. They permanently confuse the log reader.
While decompressing the short read bytes the decompressor will emit bytes into its output stream. If we were going to catch such exceptions we need to rewind both the reader and the output stream.
Flushing the writer is insufficient, although I do that too in this patch to attempt to minimize the time where a tailer might not have a complete WALedit serialization at the current end of file.
The fix is to always provide a complete segment of the compressed stream to the decompressor. A buffer and IOUtils.readFully is required for that.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Trying to understand how this actually works, are we relying on the EOFException thrown by readFully here so that upper layers in ProtofbufReader#next() handles it?
No.
Well, yes, that would happen, but that is not the fix.
There are two problems if we use the input stream directly:
- Sometimes the input stream lies about the number of available bytes
- If we do not read in the complete segment of the compressed stream, the decompressor, depending on type, will throw random exceptions, maybe IO exceptions, maybe others. These are not EOFExceptions. They permanently confuse the log reader.
While decompressing the short read bytes the decompressor will emit bytes into its output stream. If we were going to catch such exceptions and convert to EOFException we still need to rewind both the reader and the output stream. I could explore this alternative but is it less expensive than just copying in the compressed bytes first? The most common case for reading WALs will be replication's active tailing.
Flushing the writer is insufficient, although I do that too in this patch to attempt to minimize the time where a tailer might not have a complete WALedit serialization at the current end of file.
The fix is to always provide a complete segment of the compressed stream to the decompressor. A buffer and IOUtils.readFully is required for that.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Trying to understand how this actually works, are we relying on the EOFException thrown by readFully here so that upper layers in ProtofbufReader#next() handles it?
No.
Well, yes, that would happen, but that is not the fix.
There are two problems if we use the input stream directly:
- Sometimes the input stream lies about the number of available bytes. I attempted using InputStream#available to ensure that after we read in the vint of how many compressed bytes follow, that at least that many bytes are available to read, but sometimes still got short reads.
- If we do not read in the complete segment of the compressed stream, the decompressor, depending on type, will throw random exceptions, maybe IO exceptions, maybe others. These are not EOFExceptions. They permanently confuse the log reader.
While decompressing the short read bytes the decompressor will emit bytes into its output stream. If we were going to catch such exceptions and convert to EOFException we still need to rewind both the reader and the output stream. I could explore this alternative but is it less expensive than just copying in the compressed bytes first? The most common case for reading WALs will be replication's active tailing.
Flushing the writer is insufficient, although I do that too in this patch to attempt to minimize the time where a tailer might not have a complete WALedit serialization at the current end of file.
The fix is to always provide a complete segment of the compressed stream to the decompressor. A buffer and IOUtils.readFully is required for that.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Trying to understand how this actually works, are we relying on the EOFException thrown by readFully here so that upper layers in ProtofbufReader#next() handles it?
No.
Well, yes, that would happen, but that is not the fix.
There are two problems if we use the input stream directly:
- Sometimes the input stream lies about the number of available bytes. I attempted using InputStream#available to ensure that after we read in the vint of how many compressed bytes follow, that at least that many bytes are available to read, but sometimes still got short reads even when a sufficient number of bytes were alleged to be available.
- If we do not read in the complete segment of the compressed stream, the decompressor, depending on type, will throw random exceptions, maybe IO exceptions, maybe others. These are not EOFExceptions. They permanently confuse the log reader.
While decompressing the short read bytes the decompressor will emit bytes into its output stream. If we were going to catch such exceptions and convert to EOFException we still need to rewind both the reader and the output stream. I could explore this alternative but is it less expensive than just copying in the compressed bytes first? The most common case for reading WALs will be replication's active tailing.
Flushing the writer is insufficient, although I do that too in this patch to attempt to minimize the time where a tailer might not have a complete WALedit serialization at the current end of file.
The fix is to always provide a complete segment of the compressed stream to the decompressor. A buffer and IOUtils.readFully is required for that.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Trying to understand how this actually works, are we relying on the EOFException thrown by readFully here so that upper layers in ProtofbufReader#next() handles it?
No.
Well, yes, that would happen, and that is part of the fix, but is not the whole story. It's actually more important not to provide the decompression stream with insufficient input to decompress the WALedit's value.
There are two problems if we use the input stream directly:
- Sometimes the input stream lies about the number of available bytes. I attempted using InputStream#available to ensure that after we read in the vint of how many compressed bytes follow, that at least that many bytes are available to read, but sometimes still got short reads even when a sufficient number of bytes were alleged to be available.
- If we do not read in the complete segment of the compressed stream, the decompressor, depending on type, will throw random exceptions, maybe IO exceptions, maybe others. These are not EOFExceptions. They permanently confuse the log reader.
While decompressing the short read bytes the decompressor will emit bytes into its output stream. If we were going to catch such exceptions and convert to EOFException we still need to rewind both the reader and the output stream. I could explore this alternative but is it less expensive than just copying in the compressed bytes first? The most common case for reading WALs will be replication's active tailing.
Flushing the writer is insufficient, although I do that too in this patch to attempt to minimize the time where a tailer might not have a complete WALedit serialization at the current end of file.
The fix is to always provide a complete segment of the compressed stream to the decompressor. A buffer and IOUtils.readFully is required for that.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Trying to understand how this actually works, are we relying on the EOFException thrown by readFully here so that upper layers in ProtofbufReader#next() handles it?
No.
Well, yes, that would happen, and that is part of the fix, but is not the whole story. It's actually more important to avoid the case where the decompression stream has insufficient input to decompress the WALedit's value.
There are two problems if we use the input stream directly:
- Sometimes the input stream lies about the number of available bytes. I attempted using InputStream#available to ensure that after we read in the vint of how many compressed bytes follow, that at least that many bytes are available to read, but sometimes still got short reads even when a sufficient number of bytes were alleged to be available.
- If we do not read in the complete segment of the compressed stream, the decompressor, depending on type, will throw random exceptions, maybe IO exceptions, maybe others. These are not EOFExceptions. They permanently confuse the log reader.
While decompressing the short read bytes the decompressor will emit bytes into its output stream. If we were going to catch such exceptions and convert to EOFException we still need to rewind both the reader and the output stream. I could explore this alternative but is it less expensive than just copying in the compressed bytes first? The most common case for reading WALs will be replication's active tailing.
Flushing the writer is insufficient, although I do that too in this patch to attempt to minimize the time where a tailer might not have a complete WALedit serialization at the current end of file.
The fix is to always provide a complete segment of the compressed stream to the decompressor. A buffer and IOUtils.readFully is required for that.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Trying to understand how this actually works, are we relying on the EOFException thrown by readFully here so that upper layers in ProtofbufReader#next() handles it?
No.
Well, yes, that would happen, and that is part of the fix, but is not the whole story. It's actually more important to avoid the case where the decompressor has insufficient input to decompress the WALedit's value.
There are two problems if we use the input stream directly:
- Sometimes the input stream lies about the number of available bytes. I attempted using InputStream#available to ensure that after we read in the vint of how many compressed bytes follow, that at least that many bytes are available to read, but sometimes still got short reads even when a sufficient number of bytes were alleged to be available.
- If we do not read in the complete segment of the compressed stream, the decompressor, depending on type, will throw random exceptions, maybe IO exceptions, maybe others. These are not EOFExceptions. They permanently confuse the log reader.
While decompressing the short read bytes the decompressor will emit bytes into its output stream. If we were going to catch such exceptions and convert to EOFException we still need to rewind both the reader and the output stream. I could explore this alternative but is it less expensive than just copying in the compressed bytes first? The most common case for reading WALs will be replication's active tailing.
Flushing the writer is insufficient, although I do that too in this patch to attempt to minimize the time where a tailer might not have a complete WALedit serialization at the current end of file.
The fix is to always provide a complete segment of the compressed stream to the decompressor. A buffer and IOUtils.readFully is required for that.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Trying to understand how this actually works, are we relying on the EOFException thrown by readFully here so that upper layers in ProtofbufReader#next() handles it?
No.
Well, yes, that would happen, and that is part of the fix, but is not the whole story. It's actually more important to avoid the case where the decompressor has insufficient input to decompress the WALedit's value.
There are two problems if we use the input stream directly:
- If we do not read in the complete segment of the compressed stream, the decompressor, depending on type, will throw random exceptions, maybe IO exceptions, maybe others. These are not EOFExceptions. They permanently confuse the log reader.
- Sometimes the input stream lies about the number of available bytes. I attempted using InputStream#available to ensure that after we read in the vint of how many compressed bytes follow, that at least that many bytes are available to read, but sometimes still got short reads even when a sufficient number of bytes were alleged to be available. (It's also possible I made an error attempting to implement this.)
While decompressing the short read bytes the decompressor will emit bytes into its output stream. If we were going to catch such exceptions and convert to EOFException we still need to rewind both the reader and the output stream. I could explore this alternative but is it less expensive than just copying in the compressed bytes first? The most common case for reading WALs will be replication's active tailing.
Flushing the writer is insufficient, although I do that too in this patch to attempt to minimize the time where a tailer might not have a complete WALedit serialization at the current end of file.
The fix is to always provide a complete segment of the compressed stream to the decompressor. A buffer and IOUtils.readFully is required for that.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
The decompressor reads from the input stream, advancing it.
The decompressor writes to the output stream while decompressing, advancing it.
If the current decompression fails and needs to be restarted with more data, both input and output streams need to be rewound.
When tailing a log file for replication, this problem repeats constantly.
It is better to use IOUtils.readFully to ensure all compressed bytes for the value are available before invoking the codec.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
The decompressor reads from the input stream, advancing it.
The decompressor writes to the output stream while decompressing, advancing it.
If the current decompression fails because of a short read and needs to be restarted with more data, both input and output streams will have advanced due to the operation in progress, and need to be rewound, because the log reader wants to rewind to the last known good location (assuming we convert this to an EOFException).
When tailing a log file for replication, this problem repeats constantly.
It is better to use IOUtils.readFully to ensure all compressed bytes for the value are available before invoking the codec.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
The decompressor reads from the input stream, advancing it.
The decompressor writes to the output stream while decompressing, advancing it.
If the current decompression fails because of a short read and needs to be restarted with more data, both input and output streams will have advanced due to the operation in progress, and need to be rewound, because the log reader wants to rewind to the last known good location and retry (assuming we convert this to an EOFException).
When tailing a log file for replication, this problem repeats constantly.
It is better to use IOUtils.readFully to ensure all compressed bytes for the value are available before invoking the codec.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
The decompressor reads from the input stream, advancing it.
The decompressor writes to the output stream while decompressing, advancing it.
If the current decompression fails because of a short read and needs to be restarted with more data, both input and output streams will have advanced due to the operation in progress, and need to be rewound, because the log reader wants to rewind to the last known good location and retry (assuming we convert this to an EOFException).
When tailing a log file for replication, this problem repeats constantly.
It is better to use IOUtils.readFully to ensure all compressed bytes for the value are available before invoking the codec. Nothing more need be done. At least what are being read into the buffer are the compressed bytes...
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
The decompressor reads from the input stream, advancing it.
The decompressor writes to the output stream while decompressing, advancing it.
If the current decompression encounters short read -- where the input stream claimed all of the data was available but it actually wasn't -- both the input and output streams will have advanced. The codec will have read in some data, and written out some decompressed bytes. This all has to be undone because the log reader will rewind to the last known good location and retry (assuming we convert this to an EOFException).
When tailing a log file for replication, this problem repeats constantly.
It is better to use IOUtils.readFully to ensure all compressed bytes for the value are available before invoking the codec. Nothing more need be done. At least what are being read into the buffer are the compressed bytes...
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
The decompressor reads from the input stream, advancing it.
The decompressor writes to the output stream while decompressing, advancing it.
If the current decompression encounters short read -- where the input stream claimed all of the data was available but it actually wasn't* -- both the input and output streams will have advanced. The codec will have read in some data, and written out some decompressed bytes. This all has to be undone because the log reader will rewind to the last known good location and retry (assuming we convert this to an EOFException).
* - And there is the related problem of looping around available() until more bytes are available. Does that work? Without reading anyway, will the stream even make progress?
When tailing a log file for replication, this problem repeats constantly.
It is better to use IOUtils.readFully to ensure all compressed bytes for the value are available before invoking the codec. Nothing more need be done. At least what are being read into the buffer are the compressed bytes...
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
The decompressor reads from the input stream, advancing it.
The decompressor writes to the output stream while decompressing, advancing it.
If the current decompression encounters short read -- where the input stream claimed all of the data was available but it actually wasn't* -- both the input and output streams will have advanced. The codec will have read in some data, and written out some decompressed bytes. This all has to be undone because the log reader will rewind to the last known good location and retry (assuming we convert this to an EOFException).
\* \- And there is the related problem of looping around available() until more bytes are available. Does that work? Without reading anyway, will the stream even make progress?
When tailing a log file for replication, this problem repeats constantly.
It is better to use IOUtils.readFully to ensure all compressed bytes for the value are available before invoking the codec. Nothing more need be done. At least what are being read into the buffer are the compressed bytes...
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> What I had in my mind was an "intercepting" input stream that wraps this compressed input stream and keeps track of bytes read so far. It essentially does what IOUtils.readFully() does but without copying it into a buffer. It just throws EOFException when it is end of stream and readBytesSoFar < totalBytesToBeRead.
Will the wrapped input stream get more bytes available unless someone read()s?
If we have to read to get the stream to advance, is this different from IOUtils.readFully?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> What I had in my mind was an "intercepting" input stream that wraps this compressed input stream and keeps track of bytes read so far. It essentially does what IOUtils.readFully() does but without copying it into a buffer. It just throws EOFException when it is end of stream and readBytesSoFar < totalBytesToBeRead.
Will the wrapped input stream get more bytes available unless someone read()s?
If we have to read to get the stream to advance, is this different from IOUtils.readFully?
I can certainly implement an input stream that wraps another input stream and, when given an explicit amount of data to read, uses a buffer to accumulate those bytes before returning control to its caller, if you prefer. The current patch is equivalent but does it directly in WALCellCodec instead.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
The decompressor reads from the input stream, advancing it.
The decompressor writes to the output stream while decompressing, advancing it.
If the current decompression encounters short read -- where the input stream claimed all of the data was available but it actually wasn't* -- both the input and output streams will have advanced. The codec will have read in some data, and written out some decompressed bytes. This all has to be undone because the log reader will rewind to the last known good location and retry (assuming we convert this to an EOFException).
\* \- And there is the related problem of looping around available() until more bytes are available. Does that work? Without reading anyway, will the stream even make progress?
When tailing a log file for replication, this problem repeats constantly.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
The decompressor reads from the input stream, advancing it.
The decompressor writes to the output stream while decompressing, advancing it.
If the current decompression encounters short read -- where the input stream claimed all of the data was available but it actually wasn't* -- both the input and output streams will have advanced by the time the codec finally runs out of input bits and throws an exception. The codec will have read in some data, and written out some decompressed bytes. This all has to be undone because the log reader will rewind to the last known good location and retry (assuming we convert this to an EOFException).
\* \- And there is the related problem of looping around available() until more bytes are available. Does that work? Without reading anyway, will the stream even make progress?
When tailing a log file for replication, this problem repeats constantly.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
The decompressor reads from the input stream, advancing it.
The decompressor writes to the output stream while decompressing, advancing it.
If the current decompression encounters short read -- where the input stream claimed all of the data was available but it actually wasn't* -- both the input and output streams will have advanced by the time the codec finally runs out of input bits and throws an exception.
\* \- There is the related problem of looping around available() until more bytes are available. Without reading anyway, will the stream even make progress?
There is also the problem of deciding what exceptions to convert to EOFException. I don't think we want to do that for data corruption cases. If the data is corrupt rewinding and retrying repeatedly will not help. It would be better to fail out the reader right away because it will never succeed.
When tailing a log file for replication, this problem repeats constantly.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
Yeah I didn't communicate that well. The decompressor reads from the input stream, advancing it. The decompressor writes to the output stream consumed by the log reader while decompressing, advancing it. The number of bytes output exceeds the number of bytes input. If the current decompression encounters short read -- where the input stream claimed all of the data was available but it actually wasn't* -- it throws an exception and in the current code it is the job of the WAL reader to clean this up, by rewinding the input and retrying, and it becomes confused.
\* \- There is the related problem of looping around available() until more bytes are available. Without reading anyway, will the stream even make progress?
There is also the problem of deciding what exceptions to convert to EOFException. I don't think we want to do that for data corruption cases. If the data is corrupt rewinding and retrying repeatedly will not help. It would be better to fail out the reader right away because it will never succeed.
When tailing a log file for replication the possibility of partial reads of serialized WALedits at end of file is high.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> Where is this output stream that we need to rewind? Isn't that the job of the compression codec to clean up the state if the read() fails (or did I misunderstand something?)
Yeah I didn't communicate that well. The decompressor reads from the input stream, advancing it. The decompressor writes to the output stream consumed by the log reader while decompressing, advancing it. The number of bytes output exceeds the number of bytes input. If the current decompression encounters short read -- where the input stream claimed all of the data was available but it actually wasn't* -- it throws an exception and in the current code it is the job of the WAL reader to clean this up, by rewinding the input and retrying, and it becomes confused about position.
\* \- There is the related problem of looping around available() until more bytes are available. Without reading anyway, will the stream even make progress?
There is also the problem of deciding what exceptions to convert to EOFException. I don't think we want to do that for data corruption cases. If the data is corrupt rewinding and retrying repeatedly will not help. It would be better to fail out the reader right away because it will never succeed.
When tailing a log file for replication the possibility of partial reads of serialized WALedits at end of file is high.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> What I had in my mind was an "intercepting" input stream that wraps this compressed input stream and keeps track of bytes read so far. It essentially does what IOUtils.readFully() does but without copying it into a buffer. It just throws EOFException when it is end of stream and readBytesSoFar < totalBytesToBeRead.
This would almost work but the "intercepting" stream has to continue to read/retry to trigger the IO up the stream hierarchy to eventually read in totalBytesToBeRead, when they become available. This isn't a permanent EOF, it's a temporary EOF while we are waiting for some buffering somewhere in the stack to flush.
I will give this an attempt and get back to you. @bharathv
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
No, I don't understand how you want me to do this differently. We don't control the read() in the codec implementation. We can't modify them to use some new readFully() method of an input stream. If we try to hide this with the input stream implementation itself i.e. override available() as read into a buffer to ensure availability and then feed that buffer into read() until empty, we've just complicated things for no gain, there is still a buffer that is filled to ensure availability.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> essentially does what IOUtils.readFully() does but without copying it into a buffer.
This is the part that seems impossible. In order to do what IOUtils.readFully does you have to actually read(), and what is read in has to go *somewhere*, and it can't go to the codec until finished.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> essentially does what IOUtils.readFully() does but without copying it into a buffer.
This is the part that seems impossible. In order to do what IOUtils.readFully does you have to actually read(), and what is read in has to go *somewhere*, and it can't go to the codec until finished, because the codec does read()s assuming the stream has everything it needs. We get one chance to assure all bytes are available, and that is before we pass control to the decompressor. In WALCellCodec.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> essentially does what IOUtils.readFully() does but without copying it into a buffer.
This is the part that seems impossible. In order to do what IOUtils.readFully does you have to actually read(), and what is read in has to go *somewhere*, and it can't go to the codec until finished, because the codec does read()s assuming the stream has everything it needs. We get one chance to assure all bytes are available, and wait until that is the case, doing IO here and there as necessary, and that is before we pass control to the decompressor. In WALCellCodec.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
No, I don't understand how you want me to do this differently. We don't control the read() in the codec implementation. We can't modify them to use some new readFully() method of an input stream. If we try to hide this with the input stream implementation itself i.e. override available() as read into a buffer to ensure availability and then feed that buffer into read() until empty, we've just complicated things for no gain, there is still a buffer that is filled to ensure availability.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
> essentially does what IOUtils.readFully() does but without copying it into a buffer.
This is the part that seems impossible. In order to do what IOUtils.readFully does you have to actually read(), and what is read in has to go *somewhere*, and it can't go to the codec until finished, because the codec does arbitrary read()s assuming the stream has everything it needs. We get one chance to assure all bytes are available, and wait until that is the case, doing IO here and there as necessary, and that is before we pass control to the decompressor. In WALCellCodec.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
Wait.
I just found something very stupid but effective to do, and yet valid.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALCellCodec.java
##########
@@ -381,8 +385,13 @@ private static void checkLength(int len, int max) throws IOException {
private void readCompressedValue(InputStream in, byte[] outArray, int outOffset,
int expectedLength) throws IOException {
int compressedLen = StreamUtils.readRawVarint32(in);
- int read = compression.getValueCompressor().decompress(in, compressedLen, outArray,
- outOffset, expectedLength);
+ // A partial read of the compressed bytes, depending on which compression codec is used,
+ // can cause messy IO errors. This can happen when the reader is actively tailing a file
+ // being written, for replication.
+ byte[] buffer = new byte[compressedLen];
+ IOUtils.readFully(in, buffer, 0, compressedLen);
Review comment:
Wait.
I just found something very stupid but effective to do, and yet valid. No copy.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-860871637
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 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 27s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 52s | master passed |
| +1 :green_heart: | compile | 1m 24s | master passed |
| +1 :green_heart: | shadedjars | 8m 16s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 1m 0s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 40s | the patch passed |
| +1 :green_heart: | compile | 1m 25s | the patch passed |
| +1 :green_heart: | javac | 1m 25s | the patch passed |
| +1 :green_heart: | shadedjars | 8m 12s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 0m 58s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 1m 47s | hbase-common in the patch passed. |
| +1 :green_heart: | unit | 145m 31s | hbase-server in the patch passed. |
| | | 181m 3s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/3/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/3377 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 966b5d4d6e42 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / ba6995e083 |
| Default Java | AdoptOpenJDK-1.8.0_282-b08 |
| Test Results | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/3/testReport/ |
| Max. process+thread count | 4145 (vs. ulimit of 30000) |
| modules | C: hbase-common hbase-server U: . |
| Console output | https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-3377/3/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell merged pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
apurtell merged pull request #3377:
URL: https://github.com/apache/hbase/pull/3377
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] apurtell commented on pull request #3377: HBASE-25994 Active WAL tailing fails when WAL value compression is enabled
Posted by GitBox <gi...@apache.org>.
apurtell commented on pull request #3377:
URL: https://github.com/apache/hbase/pull/3377#issuecomment-858991850
Added all reviewers of original change https://github.com/apache/hbase/pull/3244
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org