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

ARTEMIS-1129: Client Dependencies #1272

Closed

Conversation

michaelandrepearce
Copy link
Contributor

Create shaded versions of the clients, so that end users have a single clean dependency to depend on.

Third party dependency's are re-packaged/relocated to avoid version / depedency issues.

@Rossi1337
Copy link

Hi,
I just give it a try. One first thing. In the shaded artemis-jms-client-all jar there is in the META-INF folder a services entry for the Json provider javax.json.spi.JsonProvider

Inside of this file the provider implementation is set to org.apache.johnzon.core.JsonProviderImpl
but I think it should point now to org.apache.activemq.artemis.shaded.org.apache.johnzon.core.JsonProviderImpl

There is a resource transformer that may solve this
https://maven.apache.org/plugins/maven-shade-plugin/examples/resource-transformers.html#ServicesResourceTransformer

Additionally when I try to run my test application with the shaded JMS client I get an exception

Caused by: ActiveMQNotConnectedException[errorType=NOT_CONNECTED message=AMQ119007: Cannot connect to server(s). Tried with all available servers.]
at org.apache.activemq.artemis.core.client.impl.ServerLocatorImpl.createSessionFactory(ServerLocatorImpl.java:787)
at org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory.createConnectionInternal(ActiveMQConnectionFactory.java:755)
... 40 more

Maybe the session factories are internally invoked via reflection? The stack trace does not really show what is going wrong internally but maybe this is only a configuration issue on my side. Need to double check this.

@michaelandrepearce
Copy link
Contributor Author

michaelandrepearce commented May 17, 2017

Re the issue.
We are able to run against remote broker fine, it be great if you can help me re-create your issue, so then i can debug.

are you running a remote client or invm client?

The invm parts aren't shaded as if you're running invm you would have the core server classes anyhow. , which is why as normal dependency these aren't there.

Also what version is broker?

Have you a code snippet of how you are constructing your connection factory.

Create shaded versions of the clients, so that end users have a single clean dependency to depend on.

Third party dependency's are re-packaged/relocated to avoid version / depedency issues.
@michaelandrepearce
Copy link
Contributor Author

michaelandrepearce commented May 17, 2017

@Rossi1337
I think I've managed to re-create your issue, i have updated the commit, with the additional change in the all pom, that fixes this. (it is as you suspect a repackage issue)

Can you re-check out and retest if it resolves your issue?

@clebertsuconic
Copy link
Contributor

Is there any way to add tests ?

@jbertram
Copy link
Contributor

Maybe use it in the examples?

@clebertsuconic
Copy link
Contributor

@jbertram all of them?

@gemmellr
Copy link
Member

Would an alternative be just offering a separate client binary convenient download, rather than just the single all-inclusive archive? Or documenting a maven command to gather all the current jars needed for a given client release? Those wouldn't really be any harder to actually use than the single jar in the end, but would avoid needing new uber jars that could end up being a little fragile given all the shading config used to create them and going much less tested than the main modules.

Setting that aside, I notice there are various profiles specifying modules in the pom, and I wondered if it is sufficient to only add these new ones in the one place as done, or if they also need added to some of the other profiles? Not sure exactly what they all do.

@michaelandrepearce
Copy link
Contributor Author

@gemmellr as a project it is i think good citizenship to be able to provide a clean artefact.
This is no different to netty, which guess what we use for the benefit of this project, the netty-all. A clear example of the benefit of a project providing these.
The actual issue here was a user not using maven and having to cherry pick all the jars he needed just to have a client, this obviously isn't the best as he raised the ticket. and IMO i agree. Thus why I'm happy addressing this for him, and others.

@clebertsuconic @jbertram do the examples run at build time on the CI build then? if so i can update these to use it.

@Rossi1337
Copy link

It is like that. We run an application server that is not OSGI so we have of course already a version of Apache commons and Google guave, ... they will collide with the version needed by artemis client and there is no easy way to resolve this. It is not only about adding one or 4 Jars to the server. It is also about version conflicts. The shading solves this very elegant.
A typical JMS application is generic. You normally just add one JMS client jar to the classpath and you can connect. With this single JAR this looks way cleaner for me.

Thanks for all your efforts this is highly appreciated I just will check out and give it a test run.

Update tests where only client used, to use the shaded single client jar.
@michaelandrepearce
Copy link
Contributor Author

Ive updated all the standard examples, that just use the client, to use the new shaded single jar client.

@clebertsuconic
Copy link
Contributor

Oh wow. Thanks. I was starting on that and you already done it. Thanks.

@gemmellr
Copy link
Member

@michaelandrepearce Which is one reason I suggested having a specific client distribution, which would give what the JIRA requested, without involving maven. Shading can indeed have benefits, and disadvantages, but a need for it wasn't really mentioned on the JIRA. If folks are happy to maintain the modules, I have no particular issue with them existing.

As an aside, I'd consider netty-all to be a little different in that it involves less fragile changes since it has no dependencies and is just offering all of netty itself in essentially the same fashion as the split jars, albeit without the OSGi bits hence artemis using/needing netty-all in some cases and netty-* in others.

@michaelandrepearce
Copy link
Contributor Author

@gemmellr actually there are some third party deps shaded in the netty one also, e.g. jctools.

@gemmellr
Copy link
Member

@michaelandrepearce ah, well there you go, I wasn't aware of that...I guess some bits of netty I've not used needs those, or maybe they are actually shaded in the split modules to begin with.

@michaelandrepearce
Copy link
Contributor Author

@clebertsuconic @jbertram with now decent test case range, using the examples, and all looks to be passing still on PR build.

Anything else to do? (@clebertsuconic, i know you'd like to me possibly squash the commits but in this instance, i'd rather keep them separate, as a lot of example tests updated, i think its cleaner to keep the commits separate. If you really want me to squash then no worries i will, just wanted to discuss)

@clebertsuconic
Copy link
Contributor

I will run some tests. Let's keep it separate.

@asfgit asfgit closed this in 7b7a782 May 17, 2017
@clebertsuconic
Copy link
Contributor

@michaelandrepearce I usually like the PR to represent a fresh commit... you work, work work, and then at the end we merge it as fresh.. at that point we squash or keep separate depending on how it makes sense.. so this is good!

merged already.. thanks a lot again!!!

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.

5 participants