-
Notifications
You must be signed in to change notification settings - Fork 24.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better support for partial buffer reads/writes in translog infrastructure #6576
Conversation
…ture Some IO api can return after writing & reading only a part of the requested data. On these rare occasions, we should call the methods again to read/write the rest of the data. This has cause rare translog corruption while writing huge documents on Windows. Closes elastic#6441
this.versionType = VersionType.fromValue(in.readByte()); | ||
} | ||
} catch (ElasticsearchException e) { | ||
throw new ElasticsearchException("failed to read [" + type + "][" + id + "]", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we catch Throwable here since we add relevant info to wrap the exception? Also, can it be an IndexShard based exception, so we also capture the index and shard id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to throwable, but I don't see where to get the index and shard id from here..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagee, see my other comment. Catching throwable is a big no-go, as it can hide Errors like OOM (even if you rethrow).
For this case, catching ElasticSearchException is better.
if (version >= 6) { | ||
this.versionType = VersionType.fromValue(in.readByte()); | ||
} | ||
} catch (Throwable t) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cathcing Throwable is always a bad idea, because it also catches InterruptedException and Errors and this causes the interrupt status to be ignored later. It also hides stuff like OOM!!!
You should be more selective in catching those Exceptions, the only one that can happen here should be IllegalStateEx and IOExeption? So please, use a multi-catch!
… deal with InterruptedException
@rmuir @uschindler pushed another commit based on your feedback. Thx. I still need to do the forbidden API. |
this.versionType = VersionType.fromValue(in.readByte()); | ||
} | ||
} catch (Exception e) { | ||
if (e instanceof InterruptedException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think InterruptedException is maybe not needed, because its checked and not in the signatures anywhere. But Exception is much better than Throwable. Thanks!
The main issue was: It hides OutOfMemoryError and this confuses users and makes reporting tools for JVM problems useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Thought we wanted to have it... removing the InterruptedException
Looks fine now. Should I retest? |
Sure. Thx!! On Mon, Jun 23, 2014 at 5:58 PM, Uwe Schindler notifications@github.com
|
OK, tested and works. I am still investigating about the while-loop: What happens if disk is full? Would Channel#write() then always return 0, so it will get endless loop? As far as I remeber, the Javadocs say, that at least 1 byte is written. Have to validate! Otherwise a disk-full would produce an endless loop. At least with this patch we should also prevent translog corrumption with disk full, because it should throw exception! |
I have no idea how to handle writes of 0 bytes... MAYBE you should do a test with a disk full (e.g. tmpfs with limited size). Otherwise patch looks fine. |
@uschindler the java docs say "A socket channel in non-blocking mode, for example, cannot write any more bytes than are free in the socket's output buffer." & "The number of bytes written, possibly zero" I think this implies that an error is indicated by an exception and we should just retry upon 0 return value? |
I think the main problem here is, that it is undefined. The Javadocs of FileChannel#read (more the interface docs @ http://docs.oracle.com/javase/7/docs/api/java/nio/channels/ReadableByteChannel.html) clearly state how it works: "It is guaranteed, however, that if a channel is in blocking mode and there is at least one byte remaining in the buffer then this method will block until at least one byte is read" - but nothing like that for write. In Lucene's NIOFSDirectory we assert on exactly this behaviour when reading from the FileChannel (which is blocking). Is the channel of the tranlog blocking or not? I am just afraid that in some cases (especially for sockets), where nothing can be written, we wait in a spinning loop until the number of bytes is > 0. |
import java.nio.channels.GatheringByteChannel; | ||
import java.nio.channels.WritableByteChannel; | ||
|
||
public abstract class Channels { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a real unittest for this class? I mean it's tested well but I'd love to have a dedicated test for that really hammers it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this class final with a default private ctor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final I get. Wondering why the default private ctor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - I get it :) final to prevent extending, private ctor to replace the abstract. Coming up.
I really think the channel API needs to go into forbidden APIs ...I also left some comments on the commit. |
@s1monw thx. I still have a todo for this one to add the channel write/read methods to the forbidden API. |
I agree with @s1monw: java.io.Closeable requires that close() must be idempotent. Otherwise patch looks fine. |
…unting during failures in snapshot creation. Added double close protection in SimpleFsTranslogFile. Using scaledRandomIntBetween in tests
@s1monw I pushed another update. |
LGTM |
…ture Some IO api can return after writing & reading only a part of the requested data. On these rare occasions, we should call the methods again to read/write the rest of the data. This has cause rare translog corruption while writing huge documents on Windows. Noteful parts of the commit: - A new Channels class with utility methods for reading and writing to channels - Writing or reading to channels is added to the forbidden API list - Added locking to SimpleFsTranslogFile - Removed FileChannelInputStream which was not used Closes #6441 , #6576
…ture Some IO api can return after writing & reading only a part of the requested data. On these rare occasions, we should call the methods again to read/write the rest of the data. This has cause rare translog corruption while writing huge documents on Windows. Noteful parts of the commit: - A new Channels class with utility methods for reading and writing to channels - Writing or reading to channels is added to the forbidden API list - Added locking to SimpleFsTranslogFile - Removed FileChannelInputStream which was not used Closes #6441 , #6576
Pushed this. Thx all for reviewing (back porting to 1.2 run into conflicts, I'll go through them later carefully and push it as well) |
Thanks @bleskes! It was a pleasure to work with you, nice work, really! |
…ture Some IO api can return after writing & reading only a part of the requested data. On these rare occasions, we should call the methods again to read/write the rest of the data. This has cause rare translog corruption while writing huge documents on Windows. Noteful parts of the commit: - A new Channels class with utility methods for reading and writing to channels - Writing or reading to channels is added to the forbidden API list - Added locking to SimpleFsTranslogFile - Removed FileChannelInputStream which was not used Closes #6441 , #6576
does this update included in the version 1.2.1? |
@SimplyWhiteMan it only made into 1.2.2 (see PR labels). Also, since 1.2.2 we have so many similar issues that I urge to upgrade to the 1.5.2 (or 1.6.0 coming soonish). Going 1.2.2 will be a bad idea... |
…ture Some IO api can return after writing & reading only a part of the requested data. On these rare occasions, we should call the methods again to read/write the rest of the data. This has cause rare translog corruption while writing huge documents on Windows. Noteful parts of the commit: - A new Channels class with utility methods for reading and writing to channels - Writing or reading to channels is added to the forbidden API list - Added locking to SimpleFsTranslogFile - Removed FileChannelInputStream which was not used Closes elastic#6441 , elastic#6576
Some IO api can return after writing & reading only a part of the requested data. On these rare occasions, we should call the methods again to read/write the rest of the data. This has cause rare translog corruption while writing huge documents on Windows.
Closes #6441
PS. I'll add java docs to the new Channels util class, but wanted to start the review process.