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-4443 support broker plugin config via broker properties #4634

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

gtully
Copy link
Contributor

@gtully gtully commented Sep 26, 2023

No description provided.

@gtully
Copy link
Contributor Author

gtully commented Sep 26, 2023

I need to revisit, registerBrokerPlugins is part of the official api and documented as the way to register/install a plugin.
I need to rework it to support a two stage add and register, but must respect that api in some way.

@gtully gtully force-pushed the _ARTEMIS-4443 branch 2 times, most recently from b105526 to 9fc661c Compare September 26, 2023 14:04
@gtully gtully marked this pull request as ready for review September 26, 2023 14:28
@gtully
Copy link
Contributor Author

gtully commented Sep 26, 2023

by making registerBrokerPlugins check for contains on each of the typed collections, it is safe to have it called twice in the case or programmatic calls before server.start. Doing it at start allows entries added to brokerPlugins to be properly registered.

@brusdev
Copy link
Member

brusdev commented Sep 28, 2023

@gtully this is a cool improvement!

What about using an annotation to avoid the suffix .class and the default getName method?

   public @interface BrokerProperty {
      enum NameType {
         PROPERTY,
         CLASS
      }
      public NameType itemName() default NameType.PROPERTY;
   }

   @Override
   @BrokerProperty(itemName = BrokerProperty.NameType.CLASS)
   public List<ActiveMQServerBasePlugin> getBrokerPlugins() {
   ...

@gtully gtully marked this pull request as draft September 28, 2023 13:33
@gtully
Copy link
Contributor Author

gtully commented Sep 28, 2023

Thanks Dom, not yet sure of the annotation gives any value, but I see that we never reuse the configuration instance on reload, and it may be that a plugin is never referenced for configuration purposes, meaning a name attribute won't be necessary, everything needs to go through init which is a single line. That can simplify the use case a lot

@gtully gtully marked this pull request as ready for review October 2, 2023 09:54
@gtully gtully merged commit cca027b into apache:main Oct 2, 2023
6 checks passed
@gtully gtully deleted the _ARTEMIS-4443 branch October 2, 2023 09:54
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.

2 participants