Skip to content
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.

Sign up for GitHub

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

Line reader #30

Merged
merged 4 commits into from
Sep 14, 2018
Merged

Line reader #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b99d4da
Remove duplicate finder plugin
Sep 11, 2018
ee4d8e7
Copy LineReader class
Sep 6, 2018
c27fb3f
Prevent the LineReader to trigger OOM on array size
Sep 7, 2018
2716e01
Throw IOException when the line is over the limit
Sep 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 1 addition & 18 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,24 +366,6 @@
</configuration>
</plugin>

<plugin>
yingsu00 marked this conversation as resolved.
Show resolved Hide resolved
<groupId>com.ning.maven.plugins</groupId>
<artifactId>maven-duplicate-finder-plugin</artifactId>
<version>1.0.4</version>
<executions>
<execution>
<id>default</id>
<phase>verify</phase>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
<configuration>
<failBuildInCaseOfConflict>true</failBuildInCaseOfConflict>
</configuration>
</plugin>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, for explanation.


<plugin>
<groupId>com.ning.maven.plugins</groupId>
<artifactId>maven-dependency-versions-check-plugin</artifactId>
Expand Down Expand Up @@ -560,6 +542,7 @@
<artifact>org.apache.hadoop:hadoop-common</artifact>
<excludes>
<exclude>*.h</exclude>
<exclude>org/apache/hadoop/util/LineReader.class</exclude>
</excludes>
</filter>
<filter>
Expand Down
Loading