Camel
  1. Camel
  2. CAMEL-4358

Refactoring of management code to remove cycles and clearly separate between API and impl

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.8.0
    • Fix Version/s: 2.9.0
    • Component/s: camel-core
    • Labels:
      None
    • Patch Info:
      Patch Available
    • Estimated Complexity:
      Unknown

      Description

      Currently the management code (org.apache.camel.management.*) contains a mix of implementation code and APIs. This leads to cycle in the management code as well as to bigger cycles involving all of camel-core.

      So I propose to do several changes:

      • move interfaces and annotations that form the API to camel.spi.management. As these are probably not used by external components or user code no compatibility classes are created
      • create new package camel.suppport for classes that implement API or SPI classes and are expected to be extended by many impl classes. classes in support may only access the camel API and SPI
      • Move EventNotifierSupport and ServiceSupport to this package. Create deprecate dummys in the old locations to avoid breaking older code
      • Move DefaultEventFactory from management to management.event

      I will post a patch shortly for review

      1. CAMEL-4358-1.patch
        151 kB
        Christian Schneider

        Activity

        Hide
        Claus Ibsen added a comment -

        Yeah that could be a good idea.

        However if we are to change the API a bit, we may consider removing the Statistic createStatistic(String name, Object owner, Statistic.UpdateMode updateMode); method from the ManagementStrategy and create the Statistics instances directly in the source code. I doubt any SPI need to create other kind of Statistic objects in a 3rd party management strategy.

        Then we keep Statistic as an internal object in the org.apache.camel.management package.

        However I am also okay for the move to spi.

        Show
        Claus Ibsen added a comment - Yeah that could be a good idea. However if we are to change the API a bit, we may consider removing the Statistic createStatistic(String name, Object owner, Statistic.UpdateMode updateMode); method from the ManagementStrategy and create the Statistics instances directly in the source code. I doubt any SPI need to create other kind of Statistic objects in a 3rd party management strategy. Then we keep Statistic as an internal object in the org.apache.camel.management package. However I am also okay for the move to spi.
        Hide
        Claus Ibsen added a comment -

        we could then turn Statistic into a class instead of interface and make it final.

        Show
        Claus Ibsen added a comment - we could then turn Statistic into a class instead of interface and make it final.
        Hide
        Christian Schneider added a comment -

        Hi Claus,

        I have changed the goal of this issue as there are more refactorings to be done in the management code. I have move the Stastic class for now but it is quite possible that we could remove the creation from the API altogether as you proposed. I will check that in a second step

        Christian

        Show
        Christian Schneider added a comment - Hi Claus, I have changed the goal of this issue as there are more refactorings to be done in the management code. I have move the Stastic class for now but it is quite possible that we could remove the creation from the API altogether as you proposed. I will check that in a second step Christian
        Hide
        Hadrian Zbarcea added a comment -

        @Christian, I reviewed your changes. Great patch, looks much cleaner. Kinda big though, I'd prefer smaller incremental changes.

        Show
        Hadrian Zbarcea added a comment - @Christian, I reviewed your changes. Great patch, looks much cleaner. Kinda big though, I'd prefer smaller incremental changes.
        Hide
        Claus Ibsen added a comment -

        It looks like you removed JmxNotificationBroadcasterAware, is there a reason for that?

        And whats the reason for moving the 3 ManagedXXX annotations to spi.management package?
        As an end user, eg developing a custom processor/bean/component I would feel a bit odd to import spi.management to add a JMX annotation to my code. Those annotations are part of the "daily" development, and fells more natural in the org.apache.camel.management or org.apache.camel package.
        The spi.management would be for 3rd party developing some custom integration with eg. a custom management software with Camel in general. A regular Camel end user shouldn't need to use the spi package.
        Luckily those 3 ManagedXXX annotations is new in Camel 2.9 so we have the luxury of being able to place them where we like it the best.

        Also it would be nice if the patch was smaller and you did this in more steps. So if you wanted to move the ServiceSupport and I think there was one other class, then doing that in a separate patch would have been nice. But I guess from this big patch its harder to split it into two now your working codebase has changed soo much.

        Anyway you are on a good way here. Just the ManagedXXX annotations I would like to be kept in a non spi package.

        Show
        Claus Ibsen added a comment - It looks like you removed JmxNotificationBroadcasterAware, is there a reason for that? And whats the reason for moving the 3 ManagedXXX annotations to spi.management package? As an end user, eg developing a custom processor/bean/component I would feel a bit odd to import spi.management to add a JMX annotation to my code. Those annotations are part of the "daily" development, and fells more natural in the org.apache.camel.management or org.apache.camel package. The spi.management would be for 3rd party developing some custom integration with eg. a custom management software with Camel in general. A regular Camel end user shouldn't need to use the spi package. Luckily those 3 ManagedXXX annotations is new in Camel 2.9 so we have the luxury of being able to place them where we like it the best. Also it would be nice if the patch was smaller and you did this in more steps. So if you wanted to move the ServiceSupport and I think there was one other class, then doing that in a separate patch would have been nice. But I guess from this big patch its harder to split it into two now your working codebase has changed soo much. Anyway you are on a good way here. Just the ManagedXXX annotations I would like to be kept in a non spi package.
        Hide
        Christian Schneider added a comment -

        @Claus: I was not sure about the spi package either. The annotations are clearly part of the API or SPI though. So I could also place them in org.apache.camel. Normally API means that you call something using an interface and SPI means that you implement an interface. On the other hand API is tpypically understood to be user faced while SPI is about 3rd party extension. As the annotations are typically used for components they could even fall in the 3rd party case but as said I am not really sure either.

        Show
        Christian Schneider added a comment - @Claus: I was not sure about the spi package either. The annotations are clearly part of the API or SPI though. So I could also place them in org.apache.camel. Normally API means that you call something using an interface and SPI means that you implement an interface. On the other hand API is tpypically understood to be user faced while SPI is about 3rd party extension. As the annotations are typically used for components they could even fall in the 3rd party case but as said I am not really sure either.
        Hide
        Christian Schneider added a comment -

        @Claus: The JmxNotificationBroadcasterAware is moved to spi.management as it is also part of the SPI. Again I am not absolutely sure about spi vs API. Perhaps we can dicuss that on dev to get a clear picture what to put where. In any case the interface should not be in the implementation package

        Show
        Christian Schneider added a comment - @Claus: The JmxNotificationBroadcasterAware is moved to spi.management as it is also part of the SPI. Again I am not absolutely sure about spi vs API. Perhaps we can dicuss that on dev to get a clear picture what to put where. In any case the interface should not be in the implementation package
        Hide
        Christian Schneider added a comment -

        Committed the patch. I will move the .spi.management package if after the discussion in dev we tend to rather want it in an api package than spi.

        Show
        Christian Schneider added a comment - Committed the patch. I will move the .spi.management package if after the discussion in dev we tend to rather want it in an api package than spi.

          People

          • Assignee:
            Christian Schneider
            Reporter:
            Christian Schneider
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development