datastax / cassandra-quarkus

An Apache Cassandra(R) extension for Quarkus
Apache License 2.0
40 stars 28 forks source link

JAVA-2782: Substitution to fix warning re: Guava's UnsignedBytes impl #17

Closed absurdfarce closed 4 years ago

absurdfarce commented 4 years ago

This PR contains the Guava substitution from https://github.com/datastax/cassandra-quarkus/pull/14

adutra commented 4 years ago

Actually it just occurred to me @absurdfarce : the fix doesn't belong in the driver either, but rather in here. We need to publish a new version of the shaded guava artifact with this substitution in place.

absurdfarce commented 4 years ago

@adutra I thought about putting this into the shaded Guava artifact but I had some concerns about that that convinced me it wasn't a good idea. I definitely agree it's a cleaner solution, however, so convince me these things don't matter and we'll do that. :)

First was the question of versioning. The shaded Guava artifact is versioned corresponding to the version of Guava it shades, which is entirely reasonable. But we don't want to just deploy a new version of the 25.1 artifact with these substitutions included since (a) we are actually changing something (and we want users to be able to access the version with and without the substitution) and (b) the artifact isn't just a shaded version of 25.1 anymore. So we need to change the version in some way.

We can't just change it to 25.1.1 or something; that seems to suggest this corresponds to a Guava version name 25.1.1, which doesn't exist. So we'd have to use some kind of qualifier, something like 25.1-graal, to clearly indicate what's going on. And we'd then have to maintain that "-graal" qualifier indefinitely going forward.

My second concern was that this forces the user to leverage this behaviour if they use our shaded Guava artifact. I liked deploying it at the driver level since it represents the "consumer" of Guava (the driver in this case) making a decision about what behaviour we want. If we do it at the Guava level every consumer gets this behaviour across the board. That may or may not be desirable, but they get no choice in the matter.

I'm not sure either of these are fatal necessarily... but there was enough uncertainty there for me to stay away from the idea.

adutra commented 4 years ago

First was the question of versioning.

Thought about that too. In hindsight, I think it is a mistake to version this project after the corresponding Guava version. I wonder if we can switch to a better versioning scheme, e.g. 1.0.0-guava25.1-jre-SNAPSHOT. @olim7t would you be OK with this change?

this forces the user to leverage this behaviour if they use our shaded Guava artifact.

Yes, but the description of the artifact is very explicit:

Shaded Guava artifact for use in the DataStax Java driver for Apache Cassandra®.

So the users are just us :-)

absurdfarce commented 4 years ago

I very much like the idea of decoupling version numbers for the shaded JAR from the underlying Guava version... but I'm somewhat skeptical about the specific convention you proposed @adutra. Specifically I'm thinking specifying the Guava version in this way is redundant. It seems to suggest that we could have the full range of semver changes (major/minor/patch) within a given version of Guava (i.e. 1.0.0-guava25.1, 1.2.0-guava25.1, 2.3.0-guava25.1). This seems extraordinarily unlikely to me.

I guess I'd argue for just using conventional semver (major.minor.patch) with a base of 1.0.0 = guava25.1 with no modifications. Our substitution would be 1.1.0. Any future Guava upgrade would almost by definition be a major update, but we can document this in a changelog or readme or something similar.

adutra commented 4 years ago

It seems to suggest that we could have the full range of semver changes (major/minor/patch) within a given version of Guava (i.e. 1.0.0-guava25.1, 1.2.0-guava25.1, 2.3.0-guava25.1). This seems extraordinarily unlikely to me.

Not really, in Maven's parlance the alphanumeric content that comes after the dash is just a qualifier. I'm using it to clearly state which original version of Guava was used in the process of producing this shaded jar. If you don't put that information in the version number, then you would have to maintain a correspondance table somewhere else, which seems impractical.

absurdfarce commented 4 years ago

If you don't put that information in the version number, then you would have to maintain a correspondance table somewhere else, which seems impractical

Agreed, but I guess I was thinking of something super-simple here. Any update in the underlying Guava version would almost by definition be a major update from a semver perspective so it seems like this just reduces to a changelog entry:

1.0.0 = start with Guava 25.1 1.1.0 = fix some stuff 2.0.0 = update to Guava 26.0

There's a bit of hand-waving there about some possible weird cases (what if we upgrade to a new Guava version which is only a minor semver update i.e. we go to Guava 25.2?) but this hardly seems like an intractable problem.

I'm not religious on the topic or anything.. but I look at that lengthy version string and keep thinking it seems like there's redundancy there.