cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Ough" <alex.o...@sungard.com>
Subject Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Date Mon, 28 Apr 2014 13:59:34 GMT


> On April 17, 2014, 8:10 p.m., Alena Prokharchyk wrote:
> > 1) Alex, I don't quite approve the fact that the responses were modified just to
support your feature. User/account of region1 has absolutely no idea of syncing that your
plugin is doing as well as he has 0 idea that it exists in multiple databases. I would suggest
to keep these parameters in the DB of your component, and reflect all the sync updates there.
> > And in the API responses account should only see actual created/removed/modified
parameters reflecting the time when the account was created/modified/removed from the very
first DB of your region.
> > 
> > Just think about your component as of a plugin running on top of CS (and that plugin
can be enabled/disabled at any time), and do the syncing w/o CS code knowing about it. CS
just has to return you all the information that originally was set through the CS (w/o the
help of your component). All extra work your component does, should be stored in your component
DB. You can add a helper table/API where you reflect the sync time between the accounts/domains/users,
I'm fine with that.
> > 
> > 2) #2 is lined with #1. Please remove all the occurances of  _rsyncMgr.update from
AccountManagerImpl. As I said, your component should act as a plugin, and you shouldn't inject
it to the CS core managers. Make all updates directly from RSyncManager after account/domain/user
is created/modified/removed in the AccountManager; code without interfering with AccountManager.
> > 
> > 3)RegionResponse.java
> > 
> > * Please add more details to the new parameter description, its not clear what it
returns now:
> > 
> > "the active of the region"
> > 
> > * add "since" annotation
> >

1) & 2) I'm a little frustrated because I think you confirmed that the updates can be
inside the Account/Domain managers to guarantee the atomicity of the transaction when we spoke
during the conference.

And the reason why the API responses need the synced datetime is because the synchronization
needs to compare which changes are later.


- Alex


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


On April 16, 2014, 7:06 p.m., Alex Ough wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20099/
> -----------------------------------------------------------
> 
> (Updated April 16, 2014, 7:06 p.m.)
> 
> 
> Review request for cloudstack.
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> This is the review request for the core changes related with #17790 that has only the
new plugin codes.
> 
> 
> Diffs
> -----
> 
>   api/src/com/cloud/event/EventTypes.java 39ef710 
>   api/src/com/cloud/user/AccountService.java 7e37b38 
>   api/src/org/apache/cloudstack/api/ApiConstants.java fdb4558 
>   api/src/org/apache/cloudstack/api/BaseCmd.java f6f21ae 
>   api/src/org/apache/cloudstack/api/command/admin/region/AddRegionCmd.java f6743ba 
>   api/src/org/apache/cloudstack/api/command/admin/region/UpdateRegionCmd.java b08cbbb

>   api/src/org/apache/cloudstack/api/response/AccountResponse.java 2e50c51 
>   api/src/org/apache/cloudstack/api/response/DomainResponse.java 0c0281e 
>   api/src/org/apache/cloudstack/api/response/RegionResponse.java 6c74fa6 
>   api/src/org/apache/cloudstack/api/response/UserResponse.java 40e1561 
>   api/src/org/apache/cloudstack/region/Region.java df64e44 
>   api/src/org/apache/cloudstack/region/RegionService.java afefcc7 
>   api/test/org/apache/cloudstack/api/command/test/RegionCmdTest.java 10c3d85 
>   client/pom.xml d8dbde7 
>   engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml
489b37d 
>   engine/schema/src/org/apache/cloudstack/multiregion/RmapVO.java PRE-CREATION 
>   engine/schema/src/org/apache/cloudstack/multiregion/RsyncVO.java PRE-CREATION 
>   engine/schema/src/org/apache/cloudstack/multiregion/dao/RmapDao.java PRE-CREATION 
>   engine/schema/src/org/apache/cloudstack/multiregion/dao/RmapDaoImpl.java PRE-CREATION

>   engine/schema/src/org/apache/cloudstack/multiregion/dao/RsyncDao.java PRE-CREATION

>   engine/schema/src/org/apache/cloudstack/multiregion/dao/RsyncDaoImpl.java PRE-CREATION

>   engine/schema/src/org/apache/cloudstack/region/RegionVO.java 608bd2b 
>   plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java
957f708 
>   plugins/pom.xml 9b391b8 
>   server/resources/META-INF/cloudstack/core/spring-server-core-managers-context.xml fc1c7e2

>   server/src/com/cloud/api/ApiDispatcher.java 95074e2 
>   server/src/com/cloud/api/ApiResponseHelper.java 38f2f0b 
>   server/src/com/cloud/api/dispatch/ParamProcessWorker.java e9bdd8b 
>   server/src/com/cloud/api/query/dao/AccountJoinDaoImpl.java ecd97c7 
>   server/src/com/cloud/api/query/dao/UserAccountJoinDaoImpl.java 923a238 
>   server/src/com/cloud/event/ActionEventUtils.java 28e5680 
>   server/src/com/cloud/multiregion/RsyncManager.java PRE-CREATION 
>   server/src/com/cloud/multiregion/RsyncManagerImpl.java PRE-CREATION 
>   server/src/com/cloud/projects/ProjectManagerImpl.java d10c059 
>   server/src/com/cloud/user/AccountManager.java 03bf842 
>   server/src/com/cloud/user/AccountManagerImpl.java 2070ee6 
>   server/src/com/cloud/user/DomainManager.java f72b18a 
>   server/src/com/cloud/user/DomainManagerImpl.java fbbe0c2 
>   server/src/org/apache/cloudstack/region/RegionManager.java 6f25481 
>   server/src/org/apache/cloudstack/region/RegionManagerImpl.java 8910714 
>   server/src/org/apache/cloudstack/region/RegionServiceImpl.java 98cf500 
>   server/test/com/cloud/api/dispatch/ParamProcessWorkerTest.java 12051a6 
>   server/test/com/cloud/user/MockAccountManagerImpl.java f373cba 
>   server/test/com/cloud/user/MockDomainManagerImpl.java 7dddefb 
>   server/test/org/apache/cloudstack/networkoffering/ChildTestConfiguration.java 22516c0

>   server/test/org/apache/cloudstack/region/RegionManagerTest.java d7bc537 
>   setup/db/db/schema-440to450.sql 2bd5386 
>   ui/scripts/regions.js 66dae8c 
> 
> Diff: https://reviews.apache.org/r/20099/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