On Thu, Nov 8, 2012 at 7:19 PM, Kelven Yang <kelven.y...@citrix.com> wrote:
>
> As part of architecture refactoring work, we are exploring some
> initiatives that would help CloudStack move toward more component-based
> architecture, one of such efforts is to standardize the way on how a
> single individual java component to be integrated into CloudStack system.
>
> CloudStack currently uses ComponentLocator to bootstrap components, it has
> done a great job while in the mean time, there are a couple of issues with
> this customized component injection framework (in my view).
>
> 1) ComponentLocator can not fully perform dependency injection, it can
> inject independent components without a problem, however, when things come
> with dependency, it will become tricky, although most of time auto-wiring
> with @Inject annotation works, but in some other time, developers will
> have to use following explicit wiring logic to make things work.
>
> @Local(value={UsageManager.class})
> public class UsageManagerImpl implements UsageManager, Runnable {
> public static final Logger s_logger =
> Logger.getLogger(UsageManagerImpl.class.getName());
>
> protected static final String DAILY = "DAILY";
> protected static final String WEEKLY = "WEEKLY";
> protected static final String MONTHLY = "MONTHLY";
>
> private static final int HOURLY_TIME = 60;
> private static final int DAILY_TIME = 60 * 24;
> private static final int THREE_DAYS_IN_MINUTES = 60 * 24 * 3;
> private static final int USAGE_AGGREGATION_RANGE_MIN = 10;
>
> --> hard-wiring logic goes here...
> private final ComponentLocator _locator =
> ComponentLocator.getLocator(UsageServer.Name, "usage-components.xml",
> "log4j-cloud_usage");
> private final AccountDao m_accountDao =
> _locator.getDao(AccountDao.class);
> private final UserStatisticsDao m_userStatsDao =
> _locator.getDao(UserStatisticsDao.class);
> private final UsageDao m_usageDao = _locator.getDao(UsageDao.class);
> private final UsageVMInstanceDao m_usageInstanceDao =
> _locator.getDao(UsageVMInstanceDao.class);
> private final UsageIPAddressDao m_usageIPAddressDao =
> _locator.getDao(UsageIPAddressDao.class);
> private final UsageNetworkDao m_usageNetworkDao =
> _locator.getDao(UsageNetworkDao.class);
> private final UsageVolumeDao m_usageVolumeDao =
> _locator.getDao(UsageVolumeDao.class);
> private final UsageStorageDao m_usageStorageDao =
> _locator.getDao(UsageStorageDao.class);
> private final UsageLoadBalancerPolicyDao m_usageLoadBalancerPolicyDao
> = _locator.getDao(UsageLoadBalancerPolicyDao.class);
> private final UsagePortForwardingRuleDao m_usagePortForwardingRuleDao
> = _locator.getDao(UsagePortForwardingRuleDao.class);
> private final UsageNetworkOfferingDao m_usageNetworkOfferingDao =
> _locator.getDao(UsageNetworkOfferingDao.class);
> private final UsageVPNUserDao m_usageVPNUserDao =
> _locator.getDao(UsageVPNUserDao.class);
> private final UsageSecurityGroupDao m_usageSecurityGroupDao =
> _locator.getDao(UsageSecurityGroupDao.class);
> private final UsageJobDao m_usageJobDao =
> _locator.getDao(UsageJobDao.class);
>
> The problem of doing so is that we create a cross-reference between
> component and the component container, it breaks the principle of being
> component-ized. In the above case, the component shouldn't really make any
> assumption of its container, and the other problem is that developers will
> sometimes be confused at when, under what situation, they can use @Inject
> to wire a component or they have to use run-time wiring logic like above
> to make it work.
>
>
> 2) AOP pattern is implicitly buried with current injection framework.
>
> AOP(Aspect Oriented Pattern) is very powerful concept, and CloudStack uses
> it a lot, @DB annotation is a very typical example. However, wiring an
> aspect is not that explicit, it buries inside those interceptors that work
> together with ComponentLocator during injection phase. Although AOP
> concept is powerful, but it better to be explicit, we need to encourage to
> component-ize the AOP aspect itself as component, although
> ComponentLocator has no problem to offer AOP support, it lacks of letting
> people do it in a more explicit way.
>
> 3) Unit test integration
> In the example in 1), to perform a unit test on the component, it involves
> with having to construct a mocked ComponentLocator, these little pieces
> add up to make unit-testing a little challenge in CloudStack.
>
+1 - this is problematic
>
> 4) CloudStack specific logic
>
> ComponentLocator also has CloudStack specific assumptions on Adapters,
> Managers, PluggableService etc, this has to move up to the business layer
> and leave this underlying layer generic and clean, if in the future people
> want to add some other cool stuff like CoolServiceGates etc, it requires
> the developer to modify ComponentLocator, this kind of possibility alone
> makes ComponentLocator itself not really a component, we should not
> encourage that.
>
> As a solution, we are exploring the possibility to adopt Spring Framework
> into CloudStack, it has the ability to address all above issues, the goal
> here is to leave all these generic concerns away from CloudStack core
> business logic, and standardize the way we program in CloudStack. The goal
> is
>
> 1) Individual component should be clean, no hard-coded assumption of any
> component container framework
> 2) System to in layered with clean boundary. No business logic leaked into
> generic plumbing layer.
> 3) AOP pattern to be explicit, represented through aspect component and
> explicit wiring of the AOP
> 4) Make unit test simple, simple and simple (Junit Spring integration has
> already done that)
>
Great Goals...
While Spring is an option, do you have any thoughts on Hugo's OSGi suggestion?
Also, the AOP example you used relates to the persistence objects. On
a related note, do you consider it a reasonable goal to switch to a
non-custom JPA framework? IIRC, Hibernate isn't license compatible
(LGPL?). How about Apache's own OpenJPA ( http://openjpa.apache.org/
)?
>
> Since 4.0 is already out of the door, I re-raise the discussion here
> again, please feel free to share your opinions
>
> Kelven
>
>
> On 10/19/12 6:14 PM, "Kelven Yang" <kelven.y...@citrix.com> wrote:
>
> >David,
> >
> >Thanks for the remind. As we are working on a feature branch(Javelin) and
> >thought not to heat-up the mailing list while everyone is still busy on
> >getting the 4.0 release out of the door.
> >
> >We are evaluating the possibility to leverage Spring Framework to
> >standardize Dependency injection/AOP pattern usages in CloudStack. Spring
> >framework is very popular to Java community and under Apache license. By
> >switching the underlying architecture plumbing to a de-facto standardized
> >framework would help attract developers to focus more on CloudStack core
> >business logic with already familiar coding practices.
> >
> >We will not extensively use full featured Spring Framework, may primarily
> >on core DI/AOP features since these are commonly used in CloudStack, we
> >also haven't decided to go with Spring annotation based approach or XML
> >based approach yet. With currently check-ins, they are annotation based
> >which I personally prefer, however, this may change, depends on how to get
> >our old code loaded nicely under Spring.
> >
> >If anyone does have a strong opinion on this, please feel free to share
> >with us.
> >
> >
> >Kelven
> >
> >On 10/19/12 3:47 PM, "David Nalley" <da...@gnsa.us> wrote:
> >
> >>This adds a number of new dependencies.
> >>
> >>This needs to explicitly be discussed before it gets committed.
> >>See below for reference:
> >>http://markmail.org/message/huevw4ur73a64b5c
> >>
> >>On Fri, Oct 19, 2012 at 6:25 PM, <kelv...@apache.org> wrote:
> >>> Updated Branches:
> >>> refs/heads/javelin a75916d45 -> 8ef9e32cf
> >>>
> >>>
> >>> Architecture refactoring - Stateless management server - Spring
> >>>Framework initiatives
> >>>
> >>>
> >>> Project:
> >>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/repo
> >>> Commit:
> >>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/commit/8ef9e
> >>>3
> >>>2c
> >>> Tree:
> >>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/tree/8ef9e32
> >>>c
> >>> Diff:
> >>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/diff/8ef9e32
> >>>c
> >>>
> >>> Branch: refs/heads/javelin
> >>> Commit: 8ef9e32cfda4b1ce138ac167caa26521bd7a3508
> >>> Parents: a75916d
> >>> Author: Kelven Yang <kelven.y...@citrix.com>
> >>> Authored: Fri Oct 19 15:24:01 2012 -0700
> >>> Committer: Kelven Yang <kelven.y...@citrix.com>
> >>> Committed: Fri Oct 19 15:24:15 2012 -0700
> >>>
> >>> ----------------------------------------------------------------------
> >>> client/WEB-INF/web.xml | 9 ++-
> >>> pom.xml | 81
> >>>+++++++++++++++
> >>> utils/conf/db.properties | 1 +
> >>> utils/src/com/cloud/utils/events/EventBus.java | 4 +-
> >>> utils/src/com/cloud/utils/events/EventBusBase.java | 5 +-
> >>> utils/src/com/cloud/utils/events/Subscriber.java | 4 +-
> >>> 6 files changed, 99 insertions(+), 5 deletions(-)
> >>> ----------------------------------------------------------------------
> >>>
> >>>
> >>>
> >>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
> >>>c
> >>>/client/WEB-INF/web.xml
> >>> ----------------------------------------------------------------------
> >>> diff --git a/client/WEB-INF/web.xml b/client/WEB-INF/web.xml
> >>> index 50f2455..c6fd30f 100644
> >>> --- a/client/WEB-INF/web.xml
> >>> +++ b/client/WEB-INF/web.xml
> >>> @@ -20,7 +20,14 @@
> >>> xsi:schemaLocation="http://java.sun.com/xml/ns/javaee
> >>>http://java.sun.com/xml/ns/javaee/web-app_2_5.xsd";
> >>> version="2.5">
> >>>
> >>> -
> >>> + <listener>
> >>> +
> >>><listener-class>org.springframework.web.context.ContextLoaderListener</l
> >>>i
> >>>stener-class>
> >>> + </listener>
> >>> + <context-param>
> >>> + <param-name>contextConfigLocation</param-name>
> >>> + <param-value>classpath:applicationContext.xml</param-value>
> >>> + </context-param>
> >>> +
> >>> <servlet>
> >>> <servlet-name>cloudStartupServlet</servlet-name>
> >>>
> >>><servlet-class>com.cloud.servlet.CloudStartupServlet</servlet-class>
> >>>
> >>>
> >>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
> >>>c
> >>>/pom.xml
> >>> ----------------------------------------------------------------------
> >>> diff --git a/pom.xml b/pom.xml
> >>> index 3c14d4e..5f63ce6 100644
> >>> --- a/pom.xml
> >>> +++ b/pom.xml
> >>> @@ -79,6 +79,7 @@
> >>> <cs.servlet.version>2.4</cs.servlet.version>
> >>> <cs.jstl.version>1.2</cs.jstl.version>
> >>>
> >>><cs.selenium.server.version>1.0-20081010.060147</cs.selenium.server.vers
> >>>i
> >>>on>
> >>> +
> >>><org.springframework.version>3.0.5.RELEASE</org.springframework.version>
> >>> <skipTests>true</skipTests>
> >>>
> >>> </properties>
> >>> @@ -169,6 +170,86 @@
> >>> <version>${cs.junit.version}</version>
> >>> <scope>test</scope>
> >>> </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-core</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-expression</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-beans</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-aop</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-context</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-context-support</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-tx</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-jdbc</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-orm</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-oxm</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-web</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-webmvc</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + </dependency>
> >>> +
> >>> + <dependency>
> >>> + <groupId>org.springframework</groupId>
> >>> + <artifactId>spring-test</artifactId>
> >>> + <version>${org.springframework.version}</version>
> >>> + <scope>test</scope>
> >>> + </dependency>
> >>> +
> >>> </dependencies>
> >>>
> >>> <build>
> >>>
> >>>
> >>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
> >>>c
> >>>/utils/conf/db.properties
> >>> ----------------------------------------------------------------------
> >>> diff --git a/utils/conf/db.properties b/utils/conf/db.properties
> >>> index 6bdb6d6..8d98119 100644
> >>> --- a/utils/conf/db.properties
> >>> +++ b/utils/conf/db.properties
> >>> @@ -24,6 +24,7 @@ cluster.servlet.port=9090
> >>> # CloudStack database settings
> >>> db.cloud.username=cloud
> >>> db.cloud.password=cloud
> >>> +db.root.password=
> >>> db.cloud.host=localhost
> >>> db.cloud.port=3306
> >>> db.cloud.name=cloud
> >>>
> >>>
> >>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
> >>>c
> >>>/utils/src/com/cloud/utils/events/EventBus.java
> >>> ----------------------------------------------------------------------
> >>> diff --git a/utils/src/com/cloud/utils/events/EventBus.java
> >>>b/utils/src/com/cloud/utils/events/EventBus.java
> >>> index 4195acd..c1b6f70 100644
> >>> --- a/utils/src/com/cloud/utils/events/EventBus.java
> >>> +++ b/utils/src/com/cloud/utils/events/EventBus.java
> >>> @@ -17,9 +17,11 @@
> >>>
> >>> package com.cloud.utils.events;
> >>>
> >>> +import java.io.Serializable;
> >>> +
> >>> public interface EventBus {
> >>> void subscribe(String subject, Subscriber subscriber);
> >>> void unsubscribe(String subject, Subscriber subscriber);
> >>>
> >>> - void publish(String subject, PublishScope scope, Object sender,
> >>>String args);
> >>> + void publish(String subject, PublishScope scope, Object sender,
> >>>Serializable args);
> >>> }
> >>>
> >>>
> >>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
> >>>c
> >>>/utils/src/com/cloud/utils/events/EventBusBase.java
> >>> ----------------------------------------------------------------------
> >>> diff --git a/utils/src/com/cloud/utils/events/EventBusBase.java
> >>>b/utils/src/com/cloud/utils/events/EventBusBase.java
> >>> index 0c135db..cd10f1d 100644
> >>> --- a/utils/src/com/cloud/utils/events/EventBusBase.java
> >>> +++ b/utils/src/com/cloud/utils/events/EventBusBase.java
> >>> @@ -17,6 +17,7 @@
> >>>
> >>> package com.cloud.utils.events;
> >>>
> >>> +import java.io.Serializable;
> >>> import java.util.ArrayList;
> >>> import java.util.HashMap;
> >>> import java.util.List;
> >>> @@ -72,7 +73,7 @@ public class EventBusBase implements EventBus {
> >>>
> >>> @Override
> >>> public void publish(String subject, PublishScope scope, Object
> >>>sender,
> >>> - String args) {
> >>> + Serializable args) {
> >>>
> >>> if(_gate.enter(true)) {
> >>>
> >>> @@ -283,7 +284,7 @@ public class EventBusBase implements EventBus {
> >>> _children.put(key, childNode);
> >>> }
> >>>
> >>> - public void notifySubscribers(String subject, Object
> >>>sender, String args) {
> >>> + public void notifySubscribers(String subject, Object
> >>>sender, Serializable args) {
> >>> for(Subscriber subscriber : _subscribers) {
> >>> subscriber.onPublishEvent(subject,
> >>>sender, args);
> >>> }
> >>>
> >>>
> >>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8ef9e32
> >>>c
> >>>/utils/src/com/cloud/utils/events/Subscriber.java
> >>> ----------------------------------------------------------------------
> >>> diff --git a/utils/src/com/cloud/utils/events/Subscriber.java
> >>>b/utils/src/com/cloud/utils/events/Subscriber.java
> >>> index 7af283b..c3baa6f 100644
> >>> --- a/utils/src/com/cloud/utils/events/Subscriber.java
> >>> +++ b/utils/src/com/cloud/utils/events/Subscriber.java
> >>> @@ -17,6 +17,8 @@
> >>>
> >>> package com.cloud.utils.events;
> >>>
> >>> +import java.io.Serializable;
> >>> +
> >>> public interface Subscriber {
> >>> - void onPublishEvent(String subject, Object sender, String
> >>>args);
> >>> + void onPublishEvent(String subject, Object sender, Serializable
> >>>args);
> >>> }
> >>>
> >
>
>
