Line reader

[yingsu00]

Throw IOException when the text line is too big and

1. The capacity of the Text object is over the VM limit for the array size;
2. The bytes to store into the Text object is over the maxLineLength limit;
3. The consumed bytes is over the maxBytesToConsume.

Case 1) would cause OOM, and the fix (the same as the second commit in this PR)
was submitted as the following PR to Apache Hadoop:
apache/hadoop#414

The fix for case 2) and 3) was behavior change so that when the line is over the configured
maxLineLength limit (suppose it's less than the VM array size limit), the LineReader
throws IOException so that the query would fail loudly. The original LineReader behavior
was to silently discard the content for a line after the maxLineLength limit.

[kokosing]

why is this removed?

[yingsu00]

@kokosing The LineReader class is duplicated and this plugin would throw error when building.

[kokosing]

But you excluded this file here: https://github.com/prestodb/presto-hadoop-apache2/pull/30/files#diff-600376dffeb79835ede4a0b285078036R546.

This plugin was added here for a reason to catch that situation. If class is duplicated, then which one will be used?

[yingsu00]

That exclude doesn't work. I don't see what this maven-duplicate-finder-plugin is used for. There are no other duplicated classes. As a similar example, the presto-hive-apache pom.xml doesn't include this maven-duplicate-finder-plugin as well. However I added it back but changed the configuration to

true

@electrum what do you think?

[electrum]

The shade plugin does a duplicate check itself. Unfortunately, it's only a warning and doesn't fail the build, but shading is complicated and doesn't change often, so it seems reasonable that anyone working on it or reviewing can review the build output. (there are lots of other things you have to review manually when shading, this is one of them)

Having the duplicate checker is problematic because it doesn't use the exclusion config from the shade plugin. It has to be configured separately, and can be misleading since you might only configure the duplicate checker and not the shade plugin and would be lead to believe the build was safe.

Hence, my recommendation is to remove it.

[kokosing]

Thanks, for explanation.

[dain]

Can you submit a PR (or issue) to Hadoop to get this change merged upstream, and then reference it in the code... basically, say, "Remove this class once xxx is resolved in Hadoop"

[yingsu00]

@kokosing Regarding the maven-duplicate-finder-plugin, as @electrum explained, is a similar plugin that does the same thing of maven-shade-plugin, and we only need one. I have again removed maven-duplicate-finder-plugin and kept the maven-shade-plugin with the exclude of LineReader class.

[electrum]

The last commit title is too long (notice how GitHub truncates it). Please see guidelines here: https://chris.beams.io/posts/git-commit/

[electrum]

Can you add test for LineReader to cover the new behavior? It should be easy to create one using ByteArrayInputStream as the input source. Ideally, the test should cover all of the places where the exception is thrown (run with coverage in your IDE and make sure all of the throw statements are hit).

[electrum]

The shade plugin does a duplicate check itself. Unfortunately, it's only a warning and doesn't fail the build, but shading is complicated and doesn't change often, so it seems reasonable that anyone working on it or reviewing can review the build output. (there are lots of other things you have to review manually when shading, this is one of them)

Having the duplicate checker is problematic because it doesn't use the exclusion config from the shade plugin. It has to be configured separately, and can be misleading since you might only configure the duplicate checker and not the shade plugin and would be lead to believe the build was safe.

Hence, my recommendation is to remove it.

[electrum]

Nit: this looks like an accidental change

[electrum]

Do we need to worry about overflow of txtLength << 1? Same question for the one below

[yingsu00]

In such case Math.max(newTxtLength, txtLength << 1) will be newTxtLength which is <= maxLineLength and won't overflow. This is the same logic in Text.setCapacity().

[electrum]

See comments, otherwise looks good

[electrum]

Can you also add a test for LineReader that passes?

[electrum]

Nit: formatting for this is off

[electrum]

This second test will never be hit, because the first one would fail first if the string is non-empty. It would be a bit better to replace both tests with this:

assertEquals(str, new Text());

This will use the equals method of Text, which works correctly. The advantage is that on a failure, it will show the results of toString for the actual and expected values, which makes it easier to debug (compared to just showing the length is different).

The same goes for the other tests.

[electrum]

This catch is not needed. Just declare the checked exception for the test method. If it throws, the test will fail.
Same goes for the rest of the methods.

[electrum]

I would write this as

String input = "Hello world! Goodbye world!\n";
...
assertEquals(str, new Text(input));

You want to make tests as simple as possible, both so they are easy to read and to avoid bugs in tests.