> 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 > >
