Skip to content

Conversation

zentol
Copy link
Contributor

@zentol zentol commented Dec 7, 2018

What is the purpose of the change

This PR fixes the bundling of hadoop classes from com.facebook.presto.hadoop:hadoop-apache2 in the presto-s3 filesystem module.

As of right now I do not really know why this fixes the issue, but intend to file an issue with MSHADE.

Brief change log

  • reorder filter definitions

Verifying this change

Manually verified. Ensure that org\apache\flink\fs\s3presto\shaded\com\facebook\presto\hadoop\ is contained in the jar, and no other contents from this jar are included (for example, nativelib).

Optionally you can also run the presto e2e test if you also include the commits from https://github.com/zentol/flink/tree/11100.

@zentol
Copy link
Contributor Author

zentol commented Dec 11, 2018

I believe I finally figured it out. This is a bug in the shade-plugin where it merges filters defined in parent/child poms by index, and not by artifact id.
So, the second filter in the parent pom defines an exclusion for all classes, and the second filter in the presto pom defines no exclusion. As a result it inherits the exclusion from the second filter in the parent pom, excluding all classes.

@zentol
Copy link
Contributor Author

zentol commented Dec 12, 2018

I have confirmed by theory and opened MSHADE-305. In the meantime we can simply disable the merging of filters, and instead default to appending them.

Only flink-fs-s3-presto should be affected by this in a significant way, as it is the only module that used inclusion filters, leaving room for an exclusion filter to be inherited that impacts the packaging.

For the remaining modules,

  • if no filter was defined the module was not affected at all as the parent filters are inherited as is
  • if a filter was defined all parent filters were disabled. They were neither appended, nor could be merged as both the parent and child filters defined exclusions, and these (apparently) are overridden by children by default.

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @zentol. LGTM. Merging.

@tillrohrmann tillrohrmann merged commit ea8373e into apache:master Dec 12, 2018
@zentol zentol deleted the 11085 branch December 18, 2018 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants