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-853 Support for exclusive consumers #1832

Closed

Conversation

michaelandrepearce
Copy link
Contributor

Support exclusive consumer
Allow default address level settings for exclusive consumer
Allow queue level setting in broker.xml
Add the ability to set queue settings via Core JMS using address. Similar to ActiveMQ 5.X
Allow for Core JMS client to define exclusive consumer using address parameters
Add tests

@michaelandrepearce
Copy link
Contributor Author

michaelandrepearce commented Jan 31, 2018

@gtully ping. I will add docs in another PR later.

Like wise will sort OpenWire to be able to set client side after(if) this gets merged. Wanted to sort supporting this on core (client and config) as first step, as a single change, to avoid it becoming a mammoth PR.

@michaelandrepearce michaelandrepearce force-pushed the ARTEMIS-853 branch 14 times, most recently from 545ed72 to c693b94 Compare February 1, 2018 09:59
@gtully
Copy link
Contributor

gtully commented Feb 1, 2018

@michaelandrepearce to make the feature work is trivial in queueimpl; to make it dynamic is a lot of code.
I would have been shooting for an address setting and pulling that value in queueImpl on creation. An all or nothing approach.
But what you have is very comprehensive :-) I just wonder if there really is a need to have the consumer create an lvq or exclusive queue. If there is, then this nails it.
For an existing openwire client the value can come from a consumer info or from a destination parameter but mostly it is the effect that is needed, not the fact that it is dynamic, ie: Because it effects every consumer having a single consumer control it is a bit imbalanced and not typical I think.

@michaelandrepearce
Copy link
Contributor Author

michaelandrepearce commented Feb 1, 2018

@gtully yes there is a use case for the client part here in our org, we have dynamic non durable shared consumers, where one pair of shared consumers needs it to be exclusive, and the other pair don't, thus why. and why i added that in the test case.

I'd rather do this fully, than partial and have to come back to it, thus it being comprehensive.

@mtaylor or anyone(PMC) with access, can someone look at the build system, ive tried 10 times, and keep getting build fails with the below which seems infra related.

ERROR: Error fetching remote repo 'origin'
hudson.plugins.git.GitException: Failed to fetch from git://github.com/apache/activemq-artemis.git

@mtaylor
Copy link
Contributor

mtaylor commented Feb 1, 2018

@michaelandrepearce Hey Michael. I think this was a temp failure. There is an Artemis PR job running now that doesn't show the git clone problem. The CI is shared and there are a bunch of jobs queued so we might have to wait a while to run again.

@michaelandrepearce
Copy link
Contributor Author

michaelandrepearce commented Feb 2, 2018

@gtully you happy if this is merged (I can do or I can’t leave for you) or you still reviewing? Just waiting on this before I continue with the other bits as mentioned

Copy link
Contributor

@gtully gtully left a comment

Choose a reason for hiding this comment

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

it looks good to me. my only suggestion would be to check out the code that does url parameter parsing in the acceptors to rationalise and possibly reuse.

@franz1981
Copy link
Contributor

@michaelandrepearce @gtully @mtaylor If everything is fine I will going to merge it, ok?

Support exlusive consumer
Allow default address level settings for exclusive consumer
Allow queue level setting in broker.xml
Add the ability to set queue settings via Core JMS using address. Similar to ActiveMQ 5.X
Allow for Core JMS client to define exclusive consumer using address parameters
Add tests
Rationalise and re-use URISupport.
@michaelandrepearce
Copy link
Contributor Author

michaelandrepearce commented Feb 5, 2018

@franz1981 seems no objections (i addressed gtully's comment on rationalising fyi) , you still ok to merge?

@franz1981
Copy link
Contributor

@michaelandrepearce i don't have the chance to merge it yet :(
Anyway it seems that there aren't any objections so I suppose it is ok 👍

@asfgit asfgit closed this in cd90bb8 Feb 7, 2018
asfgit pushed a commit that referenced this pull request Feb 7, 2018
@clebertsuconic
Copy link
Contributor

@michaelandrepearce I think the logic should have been updated on QueueImpl for the exclusive consumer...

You should just had the pos always 0. or have a different logic without using counters at all.

The code became non consistent. the pos increases but then there is a statement to always get position 0 if exclusive.

@michaelandrepearce
Copy link
Contributor Author

michaelandrepearce commented Feb 7, 2018

We do the same as if message group which doesn’t increase the pos. If it’s exclusive we don’t increment the pos like with message group

We can enhance this if needed. Ping me on IRC tomorrow? happy to make further enhancements if wanted.

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