quarkiverse / quarkus-artemis

Quarkus Artemis extensions
Apache License 2.0
13 stars 13 forks source link

Additional ActivationSpec properties for the resource adapter #400

Closed gtudan closed 6 months ago

gtudan commented 7 months ago

We have an existing application that we would like to port to quarkus. Since we don't want to change to much in the first iteration, using the resource adapter seems to be the simplest way to do it.

There are a couple of activation spec properties that are not in the ResourceAdapterFactory (yet?):

Would you accept a PR if we add them?

gtudan commented 7 months ago

I'm unsure if I should add the properties to the documentation here: https://docs.quarkiverse.io/quarkus-artemis/dev/quarkus-artemis-ra.html#_in_your_application

Adding all of the optional properties could make this confusing.

middagj commented 7 months ago

@zhfeng I assigned you to this one, as (if I remember correctly) you are the one initially working on it.

turing85 commented 7 months ago

Hmm... wrt. the documentation: we basically proxy the configurations for ActiveMQActivationSpec. I do not think that we should list everything that is settable, but link to a resource that documents what is settable. Wouldn't this readme at activemq.apache.org be the source of truth?

turing85 commented 7 months ago

@gtudan I would ask you for some patience. We would like to have the opinion of @zhfeng since he implemented the feature and is the one most familiar with resource adapters on our small team :slightly_smiling_face: . Unfortunately, he is currently unavailable. I think he should be available again at around the 19th of February.

gtudan commented 6 months ago

@zhfeng Don't want to be pushy, but could you please take a look at that PR of mine?

zhfeng commented 6 months ago

@gtudan I'm very sorry for the late reply and yeah, I will take a look.

turing85 commented 6 months ago

@louisa-fr, @middagj, @zhfeng what do you think? Should we release a new version with this feature?

gtudan commented 6 months ago

@gtudan I'm very sorry for the late reply and yeah, I will take a look.

Awesome, thanks a lot!

middagj commented 6 months ago

@louisa-fr, @middagj, @zhfeng what do you think? Should we release a new version with this feature?

Before we release can @gtudan confirm that this is working (or we can release a CR if that is easier)

gtudan commented 6 months ago

I'll give it a try and let you know

turing85 commented 6 months ago

@gtudan As soon as you can verify that the change works as expected, we can downstream the change (I have the cherry-picks ready to go) and produce new releases :slightly_smiling_face:

gtudan commented 6 months ago

Hi, I was able to validate most of the added properties ✅ share-subscriptions ✅ subscription-durability (careful, it's case sensitive) ✅ message-selector ❔subscription-name - I see that it is passed correctly in the debugger, but I'm not sure if is shown in the Artemis console somewhere

zhfeng commented 6 months ago

Thanks @gtudan for checking and I will prepare for a new release of 3.2.x

zhfeng commented 6 months ago

@gtudan 3.2.1 is out!

gtudan commented 6 months ago

Awesome, thanks for the prompt response!

turing85 commented 6 months ago

All changes are also downstreamed to 3.0.x and 3.1.x. We plan to release those changes shortly as well (probably early next week).