[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user graben commented on the issue: https://github.com/apache/activemq-artemis/pull/839 Looks good. I give it a try this evening when back home. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/839 @graben I have merged, after I reverted some of the bad code I made you do. Can you check please? And sorry about that.. I really thought the ProtocolTracker was externally used. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user graben commented on the issue: https://github.com/apache/activemq-artemis/pull/839 @clebertsuconic That's fine with me --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/839 @graben I actually just asked this on the dev-list --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/839 @graben I thought ProtocolTracker was part of the API. it's for internal use only. We could have renamed it as much as we wanted. Since I'm the one who messed up I will fix this one :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user graben commented on the issue: https://github.com/apache/activemq-artemis/pull/839 I love checkstyle. :) I thought about the actual implementation and did some small refactorings. Now I think it fits best. But we may think about synchronizing in callback as they are called in separate threads. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/839 sounds good --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user graben commented on the issue: https://github.com/apache/activemq-artemis/pull/839 Have to fix a bug introduced by refactoring. I think inner classes are looking better than an anonymous one. It might be better to add components to ProtocolTrackerCallBackImpl too make it a static class!? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/839 @graben I understand it works now. I was just trying to be more generic on the implementation and support future needs. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user graben commented on the issue: https://github.com/apache/activemq-artemis/pull/839 A bit more this way? IMO I did not see why to change the method to addDataSource. It could be only one and this one is a dependency which is mandatory for broker startup. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user graben commented on the issue: https://github.com/apache/activemq-artemis/pull/839 Rebase is done :) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/839 let me think about the class name... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/839 @graben can you rebase against master... and push -f at least? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user graben commented on the issue: https://github.com/apache/activemq-artemis/pull/839 Well, I think it must be the same callback to avoid the protocol tracker to start the broker if the datasource is not bounded yet. It's just another "mandatory" dependency. I might rename the class to more general name, but separating would end up in a mass. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/839 @graben ^^^ (<<< just to send you a notification) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/839 There is one change that is wrong here. You are using the ProtocolTrackerCallback to admin DataSources. Can you separate this into a separate class? DataSourceTrackerCallback? And instead of returning a true or false, can you return the name of the datasources? Also: I coudln't comment on the change here given the merge mess you have on this branch now. Can you execute these commands please? ``` git pull --rebase upstream master git push origin -f your-branch-name ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user mtaylor commented on the issue: https://github.com/apache/activemq-artemis/pull/839 @clebertsuconic great. It looks like @graben has addressed your comments. If you're happy I'll merge this? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user clebertsuconic commented on the issue: https://github.com/apache/activemq-artemis/pull/839 @mtaylor that merge script we always use will fix it. I already tried it --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] activemq-artemis issue #839: ARTEMIS-793 Improvement to OSGi integration
Github user mtaylor commented on the issue: https://github.com/apache/activemq-artemis/pull/839 @graben could you please squash you please rebase on top of master vs using the merge commit. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
