cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kelven Yang <kelven.y...@citrix.com>
Subject [DISCUSS] git commit: Architecture refactoring - Stateless management server - Spring Framework initiatives
Date Fri, 09 Nov 2012 00:19:43 GMT
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.

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)

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.yang@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" <david@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,  <kelveny@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.yang@citrix.com>
>>> Authored: Fri Oct 19 15:24:01 2012 -0700
>>> Committer: Kelven Yang <kelven.yang@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);
>>>  }
>>>
>


Mime
View raw message