cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alena Prokharchyk" <alena.prokharc...@citrix.com>
Subject Re: Review Request 17790: Domain-Account-User Sync Up Among Multiple Regions
Date Thu, 13 Mar 2014 19:42:02 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/17790/#review37097
-----------------------------------------------------------


Patch B.

* You shouldn’t make your service a part of cloud-mom-rabbitmq plugin. Your subscribers/implementation
are specific to your feature, and you need to introduce a special plugin just for your service.
* AccountInterface and BaseInterface are still regular classes. You should split them into
Service interface /ManagerImpl or Manager interface /ManagerImpl as Chiradeep suggested.
* Once you extract services interfaces, make sure you don’t use VO objects in methods signatures.
* You should really get a use of @Manager interface and @Inject annotations for autowiring
your managers instead of setting them up using ComponentContext.getComponent() in each of
your manager classes.



Patch A.

* AccountResponse. Why did you add domain path to the account response? You can always retrieve
it by a) get domain info from account response b) execute listDomains&domainId to get
the domain path. Try not to overload the response with attributes that don’t belong to response’s
first class object.


Generic comments.

* I can see that you do a lot of string concatenation this way: paramStr += "&email="
+ email + "&firstname=" + firstName + "&lastname=" + lastName + "&accounttype="
+ accountType;
I would suggest to use StringBuilder instead.

- Alena Prokharchyk


On March 12, 2014, 3:57 p.m., Alex Ough wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17790/
> -----------------------------------------------------------
> 
> (Updated March 12, 2014, 3:57 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Currently, under the environment of cloudstack with multiple regions, each region has
its own management server running with a separate database, which will cause data discrepancies
when users create/update/delete domain/account/user data independently in each management
server. So to support multiple regions and provide one point of entry for each customer, this
implementation duplicates domain/account/user information of customers in one region to all
of the regions independently whenever there is any change.
> 
> https://issues.apache.org/jira/browse/CLOUDSTACK-4992
> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Domain-Account-User+Sync+Up+Among+Multiple+Regions
> 
> 
> Diffs
> -----
> 
>   plugins/event-bus/rabbitmq/src/org/apache/cloudstack/mom/rabbitmq/AccountSubscriber.java
PRE-CREATION 
>   plugins/event-bus/rabbitmq/src/org/apache/cloudstack/mom/rabbitmq/DomainSubscriber.java
PRE-CREATION 
>   plugins/event-bus/rabbitmq/src/org/apache/cloudstack/mom/rabbitmq/MultiRegionEventBus.java
PRE-CREATION 
>   plugins/event-bus/rabbitmq/src/org/apache/cloudstack/mom/rabbitmq/MultiRegionSubscriber.java
PRE-CREATION 
>   plugins/event-bus/rabbitmq/src/org/apache/cloudstack/mom/rabbitmq/UserSubscriber.java
PRE-CREATION 
>   plugins/event-bus/rabbitmq/test/org/apache/cloudstack/mom/rabbitmq/AccountSubscriberTest.java
PRE-CREATION 
>   plugins/event-bus/rabbitmq/test/org/apache/cloudstack/mom/rabbitmq/DomainSubscriberTest.java
PRE-CREATION 
>   plugins/event-bus/rabbitmq/test/org/apache/cloudstack/mom/rabbitmq/MultiRegionSubscriberTest.java
PRE-CREATION 
>   plugins/event-bus/rabbitmq/test/org/apache/cloudstack/mom/rabbitmq/UserSubscriberTest.java
PRE-CREATION 
>   server/src/org/apache/cloudstack/region/multi/api/AccountInterface.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/api/BaseInterface.java PRE-CREATION 
>   server/src/org/apache/cloudstack/region/multi/api/DomainInterface.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/api/UserInterface.java PRE-CREATION 
>   server/src/org/apache/cloudstack/region/multi/service/AccountFullSyncProcessor.java
PRE-CREATION 
>   server/src/org/apache/cloudstack/region/multi/service/AccountService.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/service/BaseService.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/service/DomainFullSyncProcessor.java
PRE-CREATION 
>   server/src/org/apache/cloudstack/region/multi/service/DomainService.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/service/FullScanner.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/service/FullSyncProcessor.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/service/LocalAccountManager.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/service/LocalDomainManager.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/service/LocalUserManager.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/service/RemoteAccountEventProcessor.java
PRE-CREATION 
>   server/src/org/apache/cloudstack/region/multi/service/RemoteDomainEventProcessor.java
PRE-CREATION 
>   server/src/org/apache/cloudstack/region/multi/service/RemoteEventProcessor.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/service/RemoteUserEventProcessor.java
PRE-CREATION 
>   server/src/org/apache/cloudstack/region/multi/service/UserFullSyncProcessor.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/service/UserService.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/simulator/AccountLocalGenerator.java
PRE-CREATION 
>   server/src/org/apache/cloudstack/region/multi/simulator/AccountLocalGeneratorEvent.java
PRE-CREATION 
>   server/src/org/apache/cloudstack/region/multi/simulator/AutoGenerator.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/simulator/DomainLocalGenerator.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/simulator/DomainLocalGeneratorEvent.java
PRE-CREATION 
>   server/src/org/apache/cloudstack/region/multi/simulator/LocalGenerator.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/simulator/UserLocalGenerator.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/multi/simulator/UserLocalGeneratorEvent.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/api/AccountInterfaceTest.java PRE-CREATION

>   server/test/org/apache/cloudstack/region/multi/api/BaseInterfaceTest.java PRE-CREATION

>   server/test/org/apache/cloudstack/region/multi/api/DomainInterfaceTest.java PRE-CREATION

>   server/test/org/apache/cloudstack/region/multi/api/UserInterfaceTest.java PRE-CREATION

>   server/test/org/apache/cloudstack/region/multi/service/AccountFullSyncProcessorTest.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/service/BaseServiceTest.java PRE-CREATION

>   server/test/org/apache/cloudstack/region/multi/service/DomainFullSyncProcessorTest.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/service/RemoteAccountEventProcessorTest.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/service/RemoteDomainEventProcessorTest.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/service/RemoteUserEventProcessorTest.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/service/UserFullSyncProcessorTest.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/simulator/AccountLocalGeneratorEventTest.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/simulator/AccountLocalGeneratorTest.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/simulator/DomainLocalGeneratorEventTest.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/simulator/DomainLocalGeneratorTest.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/simulator/LocalGeneratorTest.java PRE-CREATION

>   server/test/org/apache/cloudstack/region/multi/simulator/UserLocalGeneratorEventTest.java
PRE-CREATION 
>   server/test/org/apache/cloudstack/region/multi/simulator/UserLocalGeneratorTest.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/17790/diff/
> 
> 
> Testing
> -------
> 
> 1. Successfully tested real time synchronization as soon as resources are created/deleted/modified
in one region.
> 2. Successfully tested full scans to synchronize resources that were missed during real
time synchronization because of any reasons like network connection issues.
> 3. The tests were done manually and also automatically by randomly generating changes
each region.
> 
> 
> Thanks,
> 
> Alex Ough
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message