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.

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

Move DataStax shaded Guava module into Java driver #1993

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

lukasz-antoniak
Copy link
Member

Fixes CASSJAVA-52.

@absurdfarce
Copy link
Contributor

absurdfarce commented Dec 18, 2024

So, a fun consequence of this change that I wasn't expecting (but that makes perfect sense in retrospect)... using this configuration the following things are true:

  • The build fails (with an inability to find the shaded Guava classes) if you just do "mvn test" without having previously done a "mvn install"
  • Opening this project directly in your IDE also fails, presumably for similar reasons

Why? Because "mvn test" doesn't actually build and install the local artifact... and since that's really all that's happening in guava-shaded now without the artifact there's no way to get the Guava classes into the classpath. Once you do a "mvn install" everything should be fine.

DataStax Jenkins is fine because "mvn install" is what the DataStax Jenkins build does.

I'm not necessarily saying this change should invalidate the rest of this PR... but it did seem like a significant enough change in behaviour to raise and discuss.

@absurdfarce
Copy link
Contributor

Huh, doing a "mvn install" did make "mvn test" work (as expected)... but IntelliJ still isn't find the shaded Guava classes yet. Not sure what that's about... still investigating...

<relocations>
<relocation>
<pattern>com.google</pattern>
<shadedPattern>com.datastax.oss.driver.shaded.guava</shadedPattern>
Copy link
Contributor

Choose a reason for hiding this comment

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

I get that we need this here now for backward compat reasons but... hypothetically we want to change this path to an org.apache path at some point, right?

That could be a 5.x thing I guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, especially given that all driver modules including core are still using com.datastax.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added a pointer to this conversation to CASSJAVA-62 (the "re-think how we do Guava" ticket). That's currently included on the list of things to consider as breaking changes for 5.x.

@adutra
Copy link
Contributor

adutra commented Dec 19, 2024

@absurdfarce I work currently on two projects that have a similar setup where a library is shaded within the project in a specific submodule, then that submodule is imported by other submodules in the project. The symptoms you describe are indeed common, and are generally solved by a full mvn install or gradle build. I guess that an annoyance we'll have to live with if the shaded guava module is moved inside the java driver code and starts sharing the same versioning.

@absurdfarce
Copy link
Contributor

Agreed @adutra ; I don't see anything here that should prohibit us from moving forward. Just wanted to make sure we'd considered all the implications.

I'm fine with this approach in general, and I'm pretty sure the IntelliJ issue mentioned above is something specific to my environment. Besides, if we find out we have an issue of some kind with this impl we can always either update it or (in the worst case) roll it back.

patch by Lukasz Antoniak; reviewed by Alexandre Dutra and Bret McGuire for CASSJAVA-52
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.

3 participants