Alex, please see the answers inline. May be it would be a good idea if we discuss the high level architecture - what DB changes you are going to make, how CS apis would or wouldn¹t get affected - before you do the coding? It would definitely save us time, and won¹t lead to further frustration. I¹m more than willing to help.
On 4/28/14, 6:59 AM, "Alex Ough" <alex.o...@sungard.com> wrote: > > >> 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. Perhaps we misunderstood each other. When you/me/Alex Huang talked about it at the collab, we concluded that all the changes related to synced entries, should be done in Account/domain managers of YOUR component that does account/domain sync, not the CS itself. While the current fix just does the trick of not updating the initial created/updated/modified fields, but by introducing the fields that do duplicated functionality - sync_created/sync_updated/synced_removed - but across the regions. > >And the reason why the API responses need the synced datetime is because >the synchronization needs to compare which changes are later. As I said multiple times before, your component should act as any third party component doing CS DB sync, and run all the comparisons on the layer above Account/Domain layer of the CS. No modifications should be done to AccountVO/DomainVO as well as the CS account/domain managers. The synced data should be stored in the helper VO objects of your components, and the manager of your components should perform the comparison w/o impacting the CS code. When user looks at the API response, he should see no difference whether the CS runs with multi region setup, or not. You can also utilize *DetailsVO tables of the CS (user_details for user, account_details for account, domain_details for domain) to store the Synced time, instead of creating your helper VO object. These tables are meant for storing CS object details, that can be used by third party components/services. Refer to the following FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Ability+to+have+bett er+control+over+first+class+objects+in+CS Account/Domain API responses shouldn¹t get affected either. If you utilize resourceDetails tables, the sync time will be returned when call listResourceDetails API for user/account/domain. Or your component can access those tables directly. > > >- 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.ja >>va 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-cor >>e-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/netw >>ork/contrail/management/MockAccountManager.java 957f708 >> plugins/pom.xml 9b391b8 >> >>server/resources/META-INF/cloudstack/core/spring-server-core-managers-con >>text.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 >> >> >
