Clean up translog interface #7564
Conversation
| throw e; | ||
| } catch (IOException e) { | ||
| throw new TranslogCorruptedException("translog header corrupted", e); | ||
| } |
There was a problem hiding this comment.
Is it true that any I/O exception would mean that the translog is corrupt?
There was a problem hiding this comment.
If reading the header of the translog causes an IOException, I think it's safer to abort with a corruption exception (the cause will be in the stacktrace too) than to try and continue. Additionally the corrupt index exception that Lucene's CodecUtil throws is an IOException, so I elected to wrap it in our own exception.
|
I like it left one comment |
|
@s1monw so I'm kind of concerned about one of the changes I made here, which is that we now serialize the size of the operation when sending translog operations between nodes. If it were just a size it wouldn't be a big deal, but to get the size we create a new byte array for every operation, which seems much slower. What do you think of keeping |
|
Also labeling this as a blocker for 1.4 because delaying it until 1.5 means adding another translog format since this changes it (see my previous comment). |
|
@s1monw added the NoopStreamOutput that we talked about and addressed your comment. |
| * A non-threadsafe StreamOutput that doesn't actually write the bytes to any | ||
| * stream, it only keeps track of how many bytes have been written | ||
| */ | ||
| public class NoopStreamOutput extends StreamOutput { |
|
I left some comments but mainly cosmetic |
|
Updated with your feedback, except for the "stream private" comment because I don't understand what you mean. |
clintongormley
changed the title
|
LGTM |
dakrone
force-pushed
the
translog-clean-up
branch
2 times, most recently
from
f9d1a6e
to
c390caa
Compare
get rid of readSource() entirely, it sucked, Operations should be able to provide the source themselves. No more TranslogStream headers, you are now required to pass an StreamInput or StreamOutput for all operations, which means no extra state is needed and no need to construct new versions when detecting the version. Read and write translog op sizes in TranslogStreams Previously we handled these integers outside of the translog stream itself, which was very unclean because other code had to know about reading the size, or about writing the correct header sometimes. There is some additional code in LocalIndexShardGateway to handle the legacy case for older translogs, because we need to read and discard the size in order to maintain the compatibility for the streaming operations (they did not read or write the size for 1.3.x and earlier). Additionally, we need to handle a case where the header is truncated when recovering from disk Use a NoopStreamOutput instead of byte arrays Instead of writing translog operations to a temporary byte array and then writing that byte array to the stream, we now write the operation twice, once to a No-op stream to get the size, then again to the real size. This trades a little more CPU usage for less memory usage.
|
Merged to 1.x and master. |
clintongormley
changed the title
clintongormley
added
:Distributed/Distributed
This has two major parts:
Get rid of readSource() entirely, it sucked, Operations should be able
to provide the source themselves.
No more TranslogStream headers, you are now required to pass an
StreamInput or StreamOutput for all operations, which means no extra
state is needed and no need to construct new versions when detecting the
version.
The interface now exists of 2 main methods, and one helper:
Previously we handled these integers outside of the translog stream
itself, which was very unclean because other code had to know about
reading the size, or about writing the correct header sometimes.
There is some additional code in LocalIndexShardGateway to handle the
legacy case for older translogs, because we need to read and discard the
size in order to maintain the compatibility for the streaming
operations (they did not read or write the size for 1.3.x and earlier).
Additionally, we need to handle a case where the header is truncated
when recovering from disk, so added code for that.
I'm hoping this makes working with the translog easier and less error-prone. On top of this (in a separate PR) we can add the size-reading safety to prevent rare OOMEs on corrupted translogs.