Uh oh!
There was an error while loading. Please reload this page.
HBASE-25994 Active WAL tailing fails when WAL value compression is enabled - #3377
Conversation
…abled 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. Add TestReplicationCompressedWAL and TestReplicationValueCompressedWAL. Without the WALCellCodec change TestReplicationValueCompressedWAL will fail.
apurtell
commented
Jun 10, 2021
Added all reviewers of original change #3244 |
@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). |
Apache-HBase
commented
Jun 10, 2021
🎊 +1 overall
This message was automatically generated. |
apurtell
commented
Jun 10, 2021
Checkstyle nit already addressed (c2a3a90). Whitespace nit noted, will fix if there is a round of review or at commit time. |
Apache-HBase
commented
Jun 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 10, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 11, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 11, 2021
🎊 +1 overall
This message was automatically generated. |
| // 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); |
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this comment.
Wait.
I just found something very stupid but effective to do, and yet valid. No copy.
apurtell
commented
Jun 13, 2021
@bharathv |
| } | ||
| int available = in.available(); | ||
| return (int) Math.min(available, limit - pos); | ||
| return (int) (limit - pos); |
There was a problem hiding this 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..
Apache-HBase
commented
Jun 14, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 14, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 14, 2021
💔 -1 overall
This message was automatically generated. |
…abled (#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. To avoid these issues when WAL value compression is enabled, BoundedDelegatingInputStream should assume enough bytes are available to supply a reader up to its bound. This behavior is valid per the contract of available(), which provides an _estimate_ of available bytes, and equivalent to IOUtils.readFully but without requiring an intermediate buffer. Added TestReplicationCompressedWAL and TestReplicationValueCompressedWAL. Without the WALCellCodec change TestReplicationValueCompressedWAL will fail. Signed-off-by: Bharath Vissapragada <bharathv@apache.org>
Apache-HBase
commented
Jun 15, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 15, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 15, 2021
🎊 +1 overall
This message was automatically generated. |
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.