On 11/12/12 10:58 AM, "Chip Childers" <chip.child...@sungard.com> wrote:
>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?
Our immediate focus right now is to first make interval component
development easier, allow loosely coupled component to be integrated into
CloudStack in a consistent and standard way. OSGi suggestion is a good
one, I think once we have internally sliced CloudStack into a dozen of
well encapsulated components, it will be a good option.
>
>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/
>)?
It is not that we are not considering of it, but just the amount of work
and the priorities. Dependency Injection and Persistent framework by their
own are separated projects and there are not many reasons for CloudStack
not to use mature and popular ones so that we can focus on what we ought
to. I personally prefer to see CloudStack going towards that direction,
embrace some of the best from other communities and produce the best
Orchestration Framework from our side.
It will be easier to draw Java developers to CloudStack if the system
looks familiar other than that he/she has to learn every single major
plumbing work in order to get started. From this perspective, I support to
go with other popular persist framework like openJPA, just not sure
whether or not it is a good time. But in anyway, once Spring work is in
place, switching to these alternatives will become much easier too.
>
>>
>> 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/8ef
>>>>>9e
>> >>>3
>> >>>2c
>> >>> Tree:
>>
>>>>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/tree/8ef9e
>>>>>32
>> >>>c
>> >>> Diff:
>>
>>>>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/diff/8ef9e
>>>>>32
>> >>>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/8ef9e
>>>>>32
>> >>>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/8ef9e
>>>>>32
>> >>>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.ve
>>>>>rs
>> >>>i
>> >>>on>
>> >>> +
>>
>>>>><org.springframework.version>3.0.5.RELEASE</org.springframework.versio
>>>>>n>
>> >>> <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/8ef9e
>>>>>32
>> >>>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/8ef9e
>>>>>32
>> >>>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/8ef9e
>>>>>32
>> >>>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/8ef9e
>>>>>32
>> >>>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);
>> >>> }
>> >>>
>> >
>>
>>
