> -----Original Message----- > From: Kelven Yang [mailto:kelven.y...@citrix.com] > Sent: Monday, November 12, 2012 1:31 PM > To: cloudstack-dev@incubator.apache.org > Subject: Re: [DISCUSS] git commit: Architecture refactoring - Stateless > management server - Spring Framework initiatives > > > 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.
I tried openjpa before, still have the same issue that Alex pointed out 2 years ago(http://mail-archives.apache.org/mod_mbox/openjpa-users/201011.mbox/%3caanlktimeg4gi4nx92pozyzqdsyv3oatgu6ayzwba4...@mail.gmail.com%3E), the openjpa integration with eclipse is not good at all. > > > > > >> > >> 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/8e > >>>>>f9e > >>>>>32 > >> >>>c > >> >>> Diff: > >> > >>>>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/diff/8e > >>>>>f9e > >>>>>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/8e > >>>>>f9e > >>>>>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.ContextLoaderListen > >>>>>er< > >>>>>/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-clas > >> >>>s> > >> >>> > >> >>> > >> > >>>>>http://git-wip-us.apache.org/repos/asf/incubator-cloudstack/blob/8e > >>>>>f9e > >>>>>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. > ver > >>>>>sio > >>>>>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/8e > >>>>>f9e > >>>>>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/8e > >>>>>f9e > >>>>>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/8e > >>>>>f9e > >>>>>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/8e > >>>>>f9e > >>>>>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); > >> >>> } > >> >>> > >> > > >> > >>
