----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20099/#review40690 -----------------------------------------------------------
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 - Alena Prokharchyk 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 > >
