Skip to content

Conversation

@agozhiy
Copy link
Member

@agozhiy agozhiy commented Aug 20, 2019

@arina-ielchiieva
Copy link
Member

arina-ielchiieva commented Aug 20, 2019

@agozhiy could you provide more context to this issue. Why this approach was chosen?
Why calcite java.sql.Driver is chosen over Drill's one? What if other java.sql.Driver will be present in other lib? Which one will be chosen? From the first glance, current approach looks brittle, taking into account that some other file might be chosen over Drill's one. Is there a way to merge all java.sql.Driver into one or excluding all unnecessary? Since there are no unit tests to ensure the proper java.sql.Driver is chosen, we need to make sure such issue won't pop up again.

@agozhiy
Copy link
Member Author

agozhiy commented Aug 20, 2019

@arina-ielchiieva, There are two artifacts that provide java.sql.Driver to the jar. Apparently the wrong one overwrites the right. So I excluded the file from Avatica to resolve the conflict.
Initially I tried to combine the files into one so both Avatica and Drill jdbc drivers were contained in java.sql.Driver. But as Avatica driver itself is not present in the final jar, error occurred during initialization and so no drivers were registered automatically at all. It can be fixed if swap driver names in java.sql.Driver, but this is unacceptable.
The best solution would be to include the file from a specific artifact and exclude all the others but I couldn't find a way to do so. But I agree that the current approach is not safe. I'll need to look into it a bit more.

@agozhiy
Copy link
Member Author

agozhiy commented Aug 20, 2019

From the shade plugin documentation:

From a logical perspective, includes are processed before excludes, thus it's possible to use an include to collect a set of files from the archive then use excludes to further reduce the set.

So we cannot just exclude everything and then include a specific resource. Instead, we should exclude all interfering ones, that is the approach used in the PR.

Copy link
Member

@arina-ielchiieva arina-ielchiieva left a comment

Choose a reason for hiding this comment

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

Ok, since this is the only option, let's use it.

<exclude>webapps/**</exclude>
</excludes>
</filter>
<filter>
Copy link
Member

Choose a reason for hiding this comment

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

Please add comment explaining why we need to exclude this file.

<exclude>webapps/**</exclude>
</excludes>
</filter>
<filter>
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@agozhiy agozhiy force-pushed the DRILL-7353 branch 2 times, most recently from 7ca0fc9 to 173edec Compare August 21, 2019 13:49
@arina-ielchiieva
Copy link
Member

+1, please squash the commits.

@asfgit asfgit closed this in 31a4199 Aug 27, 2019
xiangt920 pushed a commit to xiangt920/drill that referenced this pull request Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants