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

Add an ActiveMQ Artemis extension #2930

Merged
merged 6 commits into from
Aug 22, 2019
Merged

Add an ActiveMQ Artemis extension #2930

merged 6 commits into from
Aug 22, 2019

Conversation

middagj
Copy link
Contributor

@middagj middagj commented Jun 21, 2019

I have added an extension to support ActiveMQ Artemis via Core or JMS. Using AMQP via reactive messaging does not suit our needs as we want to control the acknowledgements and commits.

I have kept the CDI part small and only produce a ServerLocater (Core) and ActiveMQJMSConnectionFactory (JMS). These components should not be created again even if the connection dies for some reason. I don't know if building a framework around this (using an annotation for the listener method, create a proper producing template with serialization and transactions) is needed. It would take considerably more time, but would definitely add developer joy.

I also have chosen to support a single url property and not all the options separately. In Artemis you can specify the options there (like JDBC).

The changes for the Netty native libraries I have put in the NettyProcessor. They postpone initialization of those classes to runtime. I have disable epoll and kqueue in SubstrateVM for Artemis because of an UnsatisfiedLinkError. Others also seem to fail, e.g. Ratpack, Vert.x.

@middagj
Copy link
Contributor Author

middagj commented Jun 22, 2019

The job running on agent Hosted Agent has exceeded the maximum execution time of 150.

Is this normal? A quick inspection in the log shows that Maven downloads at very low transfer rates.

@gsmet
Copy link
Member

gsmet commented Jun 22, 2019

@middagj don't worry about it, we need to set the timeout higher.

@geoand
Copy link
Contributor

geoand commented Jun 22, 2019

I already uped the timeout as part of #2910

@geoand geoand added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Jun 22, 2019
@geoand
Copy link
Contributor

geoand commented Jun 22, 2019

@middagj could you please rebase onto the latest master to fix the conflicts?

@gastaldi
Copy link
Contributor

I wonder if this PR solves #1843?

@middagj
Copy link
Contributor Author

middagj commented Jun 22, 2019

I have rebased on master. Yes this would solve #1843, but you should still manually enable JNI.

@geoand
Copy link
Contributor

geoand commented Jun 22, 2019

@gsmet perhaps we need to add a BuildItem that enables JNI?

@cescoffier
Copy link
Member

cescoffier commented Jun 23, 2019

@middagj Just to clarify, Reactive Messaging gives you total control on the acknowledgment using Message.ack():

CompletionStage<Void> consume(Message<String> message) {
   // do something with the message
  // acknoweldge
  return message.ack();
  // you will get the next message once the previous is acknowledged (back pressure)
}

In addition, did you look at the impact on the native executable size of the runtime initialization items?

@middagj
Copy link
Contributor Author

middagj commented Jun 23, 2019

Thanks, didn't know that was possible. However, I don't see transactions being possible in the reactive messaging framework, which we would like to use.

I did not look at the impact on the native executable size. Just did a comparison by disabling the runtime initializations and excluding the epoll and kqueue libraries explicitly (needed the --allow-incomplete-classpath flag) vs the proposed solution . On a small working example the difference is around 340kB on 58MB. But this will disable epoll and kqueue also when not running native but in the VM and gives NoClassDefFoundError exceptions when running.

@emmanuelbernard
Copy link
Member

@cescoffier do you mind if I make you captain for that PR review. You're probably the one with the best view on how we want to proceed here.

@emmanuelbernard emmanuelbernard added area/reactive-messaging and removed triage/needs-rebase This PR needs to be rebased first because it has merge conflicts labels Jun 24, 2019
@middagj
Copy link
Contributor Author

middagj commented Jun 24, 2019

I don't know if the label reactive messaging really fits this PR. My goal of this PR is to add support for the Artemis Core/JMS client in Quarkus. One could already use Artemis with the reactive messaging framework via AMQP, as already demonstrated in the AMQP guide.


@PostConstruct
public void init() throws Exception {
connection = serverLocator.createSessionFactory();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure whyw e don't enlist a CDI bean being the session factory provider in the extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because a ClientSessionFactory should be regarded as connection. See Artemis documentation. If your connection breaks, the ClientSessionFactory should be created again.

Copy link
Member

Choose a reason for hiding this comment

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

@middagj OK but that feels like a terrible developer experience.
The extension should make an effort to retry this connection itself. You can imagine very few users would think of doing it otherwise. Any advice on how to do it with CDI @mkouba

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, if I understand it correctly we could create bean that implements ClientSessionFactory and delegates to a factory created by serverLocator.createSessionFactory(). Then in every call we could verify the underlying factory and if something is wrong, recreate the factory again? @middagj @emmanuelbernard But I don't know anything about Artemis. So this is just a guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the developer joy can be greatly improved. I would like to not have to worry about a connection at all and just want to define a message listener and have a producer bean which can be used to send new messages in the same transaction if configured. I don't know when all that would realistic to add on, and having the ability to use Artemis client in Quarkus already helps us and probably others already.

Sounds like a good solution. There are different session factories which can be created, but that can be put in a config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying this approach, but I reconsider if this is the right way. ClientSessionFactory implements AutoCloseable and it resembles the normal JMS Connection class. If you look at both the JMS and Core ArtemisProducerManager, they look quite similar. Both protocols have the distinction between a Connection and a Session, only Core protocol names the Connection a SessionFactory.

I think it would be better to focus on JMS and create something similar like Spring's JmsListener and JmsTemplate to improve developer joy. I prefer to do it in a separate pull request as this request already enables people to add the dependency in a native image.

@emmanuelbernard
Copy link
Member

Yes don't worry too much about the label name here.

@emmanuelbernard
Copy link
Member

emmanuelbernard commented Jun 25, 2019 via email

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

I've made one small suggestion.

The rest looks good. I would like to have the opinion of @dmlloyd and @stuartwdouglas on the Class.forName and the runtime initialization.

Side notes:

  1. As you are exposing the ConnectionFactory, I can now implement the JMS support in reactive messaging which will use the produced ConnectionFactory
  2. About transaction, it's coming, but far from being perfect as XA is a blocking protocol. Right now the solution is to off-load to a thread pool. But nevertheless, the JMS connector need a ConnectionFactory and I can use yours.

@middagj
Copy link
Contributor Author

middagj commented Jun 25, 2019

If I have time today I will see if I can give an alternative for the Class.forName as suggested by @emmanuelbernard.

Perhaps it is another topic, but XA transactions aren't necessary to use JMS transactions.

@dmlloyd
Copy link
Member

dmlloyd commented Jun 25, 2019

Every single messaging and database API having its own transaction API is not a feature ;)

/cc @mmusgrov et al

@emmanuelbernard
Copy link
Member

emmanuelbernard commented Jun 25, 2019 via email

@dmlloyd
Copy link
Member

dmlloyd commented Jun 25, 2019

On Tue, Jun 25, 2019 at 4:15 PM David M. Lloyd @.***> wrote: Every single messaging and database API having its own transaction API is not a feature ;) /cc @mmusgrov https://github.com/mmusgrov et al
Why do you say that? Or rather to which part of that PR or discussion are you referring to?

I've just shamelessly co-opted the discussion to support a point in a completely separate conversation ;)

@dmlloyd
Copy link
Member

dmlloyd commented Jul 10, 2019

This change now has conflicts. Could you please rebase?

@middagj
Copy link
Contributor Author

middagj commented Jul 11, 2019

I have fixed the conflicts and also fixed the requested changes by @cescoffier, any things that should be added for this pull request to be merged?

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Added a comment about the availability of JniBuildItem.

integration-tests/artemis-core/pom.xml Outdated Show resolved Hide resolved
@middagj
Copy link
Contributor Author

middagj commented Jul 31, 2019

@gsmet I addressed your requested changes (and rebased again because of conflicts). Any other stuff that is blocking this pull request?

@gsmet
Copy link
Member

gsmet commented Aug 19, 2019

@cescoffier could you drive this puppy home?

We need a rebase but, apart from that, could you check if it's ready to go in?

@cescoffier
Copy link
Member

@gsmet The rest was ok to be merged.

@cescoffier cescoffier merged commit 4f9cbc6 into quarkusio:master Aug 22, 2019
@cescoffier cescoffier added this to the 0.22.0 milestone Aug 22, 2019
@middagj
Copy link
Contributor Author

middagj commented Sep 16, 2019

Did I forgot to add something that should result in the extension to be available in Maven central? I cannot find it after the release of 0.22.0.

@gsmet
Copy link
Member

gsmet commented Sep 16, 2019

0.22.0 is 0.21.2 + the Spring extensions + bug fixes

This will be released as part of 0.23.0, which we hope to release soon.

@middagj
Copy link
Contributor Author

middagj commented Sep 16, 2019

Thanks!

@gastaldi
Copy link
Contributor

@middagj I couldn't find any documentation regarding this extension, could you add some before 0.23.0 is released (tomorrow) ? Thanks :)

@middagj
Copy link
Contributor Author

middagj commented Sep 24, 2019

Yes, it was on my todo list. I will add some today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants