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.

Sign up for GitHub

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

Jump to bottom

ARTEMIS-2565 - Add plugin support for Federated Queues/Addresses #2903

Merged
merged 1 commit into from
Jan 6, 2020
Merged

ARTEMIS-2565 - Add plugin support for Federated Queues/Addresses #2903

merged 1 commit into from
Jan 6, 2020

Conversation

cshannon
Copy link
Contributor

@cshannon cshannon commented Dec 2, 2019

Add a new interface called ActiveMQServerFederationPlugin to allow
customization of the federated queue/address feature of the broker

@cshannon cshannon changed the title ARTEMIS-2565 - Add plugin supported for Federated Queues/Addresses ARTEMIS-2565 - Add plugin support for Federated Queues/Addresses Dec 2, 2019
@cshannon
Copy link
Contributor Author

cshannon commented Dec 9, 2019

@clebertsuconic or @michaelandrepearce - Can one of you take a look and see if it's ok to merge this?

@michaelandrepearce
Copy link
Contributor

@cshannon was out last week, will aim to look this week for you

@cshannon
Copy link
Contributor Author

cshannon commented Dec 9, 2019

@michaelandrepearce - Sounds good, thanks

michaelandrepearce
michaelandrepearce reviewed Dec 14, 2019
View reviewed changes
...src/main/java/org/apache/activemq/artemis/core/server/federation/FederatedQueueConsumer.java Outdated
try {
server.callBrokerFederationPlugins(plugin -> plugin.beforeFederatedQueueConsumerMessageHandled(this, clientMessage));
} catch (ActiveMQException t) {
logger.warn("Error executing beforeFederatedQueueConsumerMessageHandled plugin method: {}", t.getMessage(), t);
Copy link
Contributor

Choose a reason for hiding this comment

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

should be a logger with code, e.g. explicit method logger with code as at warn (same for all logger.warns in this PR)

michaelandrepearce
michaelandrepearce reviewed Dec 14, 2019
View reviewed changes
...erver/src/main/java/org/apache/activemq/artemis/core/server/federation/FederationStream.java Outdated
protected void callFederationStreamStartedPlugins() {
if (server.hasBrokerFederationPlugins()) {
try {
server.callBrokerFederationPlugins(plugin -> plugin.federationStreamStarted(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to pass this to the plugin, we should extract interface, and method of federationStreamStarted should take the interface, not the implementation. As plugin should just have api's, else will cause issues in any future refactor (like you had done in last pr)

michaelandrepearce
michaelandrepearce reviewed Dec 14, 2019
View reviewed changes
...rver/src/main/java/org/apache/activemq/artemis/core/server/federation/FederatedAbstract.java
if (server.hasBrokerFederationPlugins()) {
try {
final FederatedQueueConsumer finalConsumer = remoteQueueConsumer;
server.callBrokerFederationPlugins(plugin -> plugin.afterCreateFederatedQueueConsumer(finalConsumer));
Copy link
Contributor

Choose a reason for hiding this comment

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

we should extract interface of FederationQueueConsumer, and make plugin use the interface, not concrete implementation, as this plugins will cause it to be a public api for end users, making any future refactoring impossible.

michaelandrepearce
michaelandrepearce reviewed Dec 14, 2019
View reviewed changes
...rver/src/main/java/org/apache/activemq/artemis/core/server/federation/FederatedAbstract.java
@@ -104,9 +109,27 @@ public synchronized void createRemoteConsumer(FederatedConsumerKey key, Transfor
if (started) {
FederatedQueueConsumer remoteQueueConsumer = remoteQueueConsumers.get(key);
if (remoteQueueConsumer == null) {
if (server.hasBrokerFederationPlugins()) {
try {
server.callBrokerFederationPlugins(plugin -> plugin.beforeCreateFederatedQueueConsumer(key));
Copy link
Contributor

Choose a reason for hiding this comment

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

extract interface, so that plugin does not expose concrete implementation, but simply api.

@michaelandrepearce
Copy link
Contributor

michaelandrepearce commented Dec 14, 2019 •
edited
Loading

@cshannon as you would have seen, two main bits to address here.

  1. logging anything that is info, warn, error should always have a logging code, and use a specific log method.

  2. As plugins make things public, then interfaces should be extracted, so plugin takes interface, not the concrete implementation. Also should only expose what's needed e.g. getter fields methods, but maybe not every method?. Else will make any future refactoring on concrete classes, like just done in previous PR impossible.

Apart from that looks good. (thumbs up)

@cshannon
Copy link
Contributor Author

cshannon commented Dec 14, 2019 •
edited
Loading

@michaelandrepearce - Those changes sound good to me and I can get to it either today or Monday probably.

As a preview, 2 more things I'm working on:

  1. I have a prototype fix I'm still testing and polishing for that other bug I had found that caused extra consumers to get created. It's a bit complicated in terms of the problem so I will write up something in the Jira to try and describe the issue and solution well when I think it's ready for a PR.

  2. I have another prototype patch to support divert bindings instead of just queue bindings over federated addresses and queues. I still need to write tests for it and polish and then can do the PR.

@cshannon
Copy link
Contributor Author

@michaelandrepearce - I'm hoping to get back to this tomorrow/friday and finish it up before I am out of the office for the holidays. I had to take care of some other stuff this week that came up.

@cshannon
Copy link
Contributor Author

@michaelandrepearce - not sure if you saw or not but I updated the PR with the changes

@michaelandrepearce
Copy link
Contributor

Sorry away on PTO. Lgtm. Feel free to merge

@cshannon
ARTEMIS-2565 - Add plugin support for Federated Queues/Addresses
fe66506 
Add a new interface called ActiveMQServerFederationPlugin to allow
customization of the federated queue/address feature of the broker
@cshannon
Copy link
Contributor Author

cshannon commented Jan 6, 2020

@michaelandrepearce - sounds good, I will rebase and merge today

@asfgit asfgit closed this in 3743bc9 Jan 6, 2020
@asfgit asfgit merged commit fe66506 into apache:master Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelandrepearce michaelandrepearce michaelandrepearce left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cshannon @michaelandrepearce @asfgit