Shade Guava manually

[leventov]

It's suggested to create a module called "guava" in Druid that copies the Guava source tree (without tests) and prohibit using the real Guava in Druid source code via Checkstyle.

Shading Guava automatically using a plugin is not possible because Druid's dependencies depend on older Guava versions that are binary-incompatible with the freshest Guava. Repackaging all such dependencies doesn't seem to be a feasible task.

One could say that copying source code of a library is bad because we won't receive updates and bug fixes that happen in the upstream, but we aren't receiving them currently anyway, using the ancient 16.0.1 version!

[gianm]

Switching to a manually "shaded" version might even make it easier to keep our Guava version up to date, if we do the copy with a script that is easy to run against new Guava sources. The script could potentially copy just the Guava files we know we need and do minimal modifications to them in order to get them to be in different packages.

I think it's probably a good idea. We have so many problems caused by the combination of the Guava team's historical attitude towards backwards compatibility and the reluctance of the Hadoop community to upgrade to a non-ancient version of Guava. Recently the Guava team has had a more pragmatic attitude towards backwards compatibility (they have promised to support non-Beta APIs indefinitely). But sadly, this doesn't help us much, given that we still need to be compatible with versions from the older era.

[drcrallen]

Why not just polish off the last things that make it incompatible with newer or older guava versions?

[gianm]

Why not just polish off the last things that make it incompatible with newer or older guava versions?

Do you mean upgrading our bundled Guava to the latest version, and sticking to only using APIs that exist in both the latest version and in Guava 11?

[drcrallen]

Keep the bundled at what it is. But make all functions we actually call highly compatible. That way installations can, if desired, substitute in their own guava version.

[gianm]

If we only call highly compatible functions, why not update to the latest? Maybe some of those functions have 'better' implementations in the latest version.

[drcrallen]

Yes, I think that should be an option, but it will likely break hadoop

[gianm]

Ah does that mean you are proposing: keep ourselves at Guava 16.0.1, but reduce how much we actually use from Guava (by copying some APIs into Druid itself or by using alternative APIs) until we get down to a set that works with everything from Guava 11 to the latest?

[drcrallen]

Well put! yes

[gianm]

It sounds like @leventov's suggestion, then, is just a more extreme version of what you're suggesting: do copy-or-switch-to-alternate for all Guava APIs, not just ones that have some known incompatibility.

I guess to me, it doesn't really matter which one we do, as long as it achieves the goal of making Guava dependency problems a thing of the past. The extreme version sounds like it's more likely to do that and would be easier to implement. Are there advantages for the less extreme version?

[drcrallen]

The less extreme version allows us to be more cognizant of what we do and do not force versions in deployments. For example:

https://github.com/google/guava/releases/tag/v23.6.1 has some vulnerability fixes.

If we shade in a version, it makes operators unable to track down if they also have the vulnerability, and makes them near powerless to change libraries if they do.

[drcrallen]

See #6948 for example

[leventov]

To me it seems that there is a problem with Charles's approach (in #6948 and #6815) (also we have Closer and maybe some other classes copied from Guava already): this is very ad-hoc, each class is copied independently. There is no list of them all. Each class should be updated in separation. The likely result is that we probably won't update them.

[drcrallen]

I started with classloader reflection approaches, but people asked instead that the classes be copied over. (See the multiple declarations of support for copying the code in #6815 ).

I couldn't find a strong enough argument to refute it, and having the code in the codebase that we critically rely on has proven pretty sustainable in the past.

[jihoonson]

@drcrallen I'm still not sure what are the benefits of the reflection approach for this issue. Would you elaborate more?