Better support for partial buffer reads/writes in translog infrastructure #6576

bleskes · 2014-06-20T13:39:42Z

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.

…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

kimchy · 2014-06-20T13:46:25Z

src/main/java/org/elasticsearch/index/translog/Translog.java

+                    this.versionType = VersionType.fromValue(in.readByte());
+                }
+            } catch (ElasticsearchException e) {
+                throw new ElasticsearchException("failed to read [" + type + "][" + id + "]", e);


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?

I changed it to throwable, but I don't see where to get the index and shard id from here..

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.

uschindler · 2014-06-20T16:32:24Z

src/main/java/org/elasticsearch/index/translog/Translog.java

+                if (version >= 6) {
+                    this.versionType = VersionType.fromValue(in.readByte());
+                }
+            } catch (Throwable t) {


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

bleskes · 2014-06-23T15:14:29Z

@rmuir @uschindler pushed another commit based on your feedback. Thx.

I still need to do the forbidden API.

uschindler · 2014-06-23T15:38:18Z

src/main/java/org/elasticsearch/index/translog/Translog.java

+                    this.versionType = VersionType.fromValue(in.readByte());
+                }
+            } catch (Exception e) {
+                if (e instanceof InterruptedException) {


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.

OK. Thought we wanted to have it... removing the InterruptedException

uschindler · 2014-06-23T15:57:57Z

Looks fine now. Should I retest?

bleskes · 2014-06-23T16:01:14Z

Sure. Thx!!

On Mon, Jun 23, 2014 at 5:58 PM, Uwe Schindler notifications@github.com
wrote:

Looks fine now. Should I retest?

Reply to this email directly or view it on GitHub:
#6576 (comment)

uschindler · 2014-06-23T16:53:10Z

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!

uschindler · 2014-06-23T16:58:40Z

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.

bleskes · 2014-06-24T06:45:39Z

@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?

uschindler · 2014-06-24T08:28:52Z

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.

s1monw · 2014-06-24T10:28:16Z

src/main/java/org/elasticsearch/common/io/Channels.java

+import java.nio.channels.GatheringByteChannel;
+import java.nio.channels.WritableByteChannel;
+
+public abstract class Channels {


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.

can we make this class final with a default private ctor?

final I get. Wondering why the default private ctor?

Ah - I get it :) final to prevent extending, private ctor to replace the abstract. Coming up.

s1monw · 2014-06-24T10:30:34Z

I really think the channel API needs to go into forbidden APIs ...I also left some comments on the commit.

bleskes · 2014-06-24T10:38:21Z

@s1monw thx. I still have a todo for this one to add the channel write/read methods to the forbidden API.

uschindler · 2014-06-30T14:40:57Z

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

bleskes · 2014-07-01T07:27:06Z

@s1monw I pushed another update.

s1monw · 2014-07-01T07:34:58Z

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

bleskes · 2014-07-01T17:17:06Z

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)

uschindler · 2014-07-01T19:41:52Z

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

SimplyWhiteMan · 2015-06-08T16:57:15Z

does this update included in the version 1.2.1?

bleskes · 2015-06-08T19:51:22Z

@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

bleskes added v1.3.0 labels Jun 20, 2014

bleskes mentioned this pull request Jun 20, 2014

ElasticsearchIllegalArgumentException No version type match #6441

Closed

kimchy reviewed Jun 20, 2014
View reviewed changes

First feedback round

f311fac

uschindler reviewed Jun 20, 2014
View reviewed changes

areek assigned rmuir and unassigned rmuir Jun 23, 2014

Writing bytebuffer in chunks. Don't catch throwable but exception and…

e7d0b6b

… deal with InterruptedException

uschindler reviewed Jun 23, 2014
View reviewed changes

bleskes added 2 commits June 23, 2014 17:41

One more optimization and a bug fix

0e61af8

no longer InterruptedException handling.

63b680f

s1monw reviewed Jun 24, 2014
View reviewed changes

s1monw removed the review label Jun 24, 2014

s1monw removed the review label Jun 30, 2014

bleskes added 2 commits June 30, 2014 21:48

made Channel class final, with a private constructor. improved ref co…

18b7e41

…unting during failures in snapshot creation. Added double close protection in SimpleFsTranslogFile. Using scaledRandomIntBetween in tests

Success pattern

8230cd2

bleskes added the review label Jul 1, 2014

s1monw removed the review label Jul 1, 2014

bleskes added the bug label Jul 1, 2014

bleskes closed this Jul 1, 2014

bleskes mentioned this pull request Jul 2, 2014

'IllegalArgumentException: No type mapped for [105]' on get document #6665

Closed

bleskes deleted the partial_reads_writes branch July 2, 2014 08:35

bleskes changed the title ~~Better support for partial buffer reads/writes in translog infrastructure~~ [Translog] Better support for partial buffer reads/writes in translog infrastructure Jul 7, 2014

clintongormley changed the title ~~[Translog] Better support for partial buffer reads/writes in translog infrastructure~~ Internal: [Translog] Better support for partial buffer reads/writes in translog infrastructure Jul 9, 2014

clintongormley changed the title ~~Internal: [Translog] Better support for partial buffer reads/writes in translog infrastructure~~ Indexing: [Translog] Better support for partial buffer reads/writes in translog infrastructure Jul 16, 2014

clintongormley added the :Translog label Jun 7, 2015

clintongormley changed the title ~~Indexing: [Translog] Better support for partial buffer reads/writes in translog infrastructure~~ Better support for partial buffer reads/writes in translog infrastructure Jun 7, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better support for partial buffer reads/writes in translog infrastructure #6576

Better support for partial buffer reads/writes in translog infrastructure #6576

bleskes commented Jun 20, 2014

kimchy Jun 20, 2014

bleskes Jun 20, 2014

uschindler Jun 20, 2014

uschindler Jun 20, 2014

bleskes commented Jun 23, 2014

uschindler Jun 23, 2014

bleskes Jun 23, 2014

uschindler commented Jun 23, 2014

bleskes commented Jun 23, 2014

Looks fine now. Should I retest?

uschindler commented Jun 23, 2014

uschindler commented Jun 23, 2014

bleskes commented Jun 24, 2014

uschindler commented Jun 24, 2014

s1monw Jun 24, 2014

s1monw Jun 30, 2014

bleskes Jun 30, 2014

bleskes Jun 30, 2014

s1monw commented Jun 24, 2014

bleskes commented Jun 24, 2014

uschindler commented Jun 30, 2014

bleskes commented Jul 1, 2014

s1monw commented Jul 1, 2014

bleskes commented Jul 1, 2014

uschindler commented Jul 1, 2014

SimplyWhiteMan commented Jun 8, 2015

bleskes commented Jun 8, 2015

Better support for partial buffer reads/writes in translog infrastructure #6576

Better support for partial buffer reads/writes in translog infrastructure #6576

Conversation

bleskes commented Jun 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleskes commented Jun 23, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschindler commented Jun 23, 2014

bleskes commented Jun 23, 2014

Looks fine now. Should I retest?

uschindler commented Jun 23, 2014

uschindler commented Jun 23, 2014

bleskes commented Jun 24, 2014

uschindler commented Jun 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw commented Jun 24, 2014

bleskes commented Jun 24, 2014

uschindler commented Jun 30, 2014

bleskes commented Jul 1, 2014

s1monw commented Jul 1, 2014

bleskes commented Jul 1, 2014

uschindler commented Jul 1, 2014

SimplyWhiteMan commented Jun 8, 2015

bleskes commented Jun 8, 2015