when & how do you want to have it?
On Mon, Apr 28, 2014 at 1:18 PM, Alena Prokharchyk < alena.prokharc...@citrix.com> wrote: > 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 > >> > >> > > > > >
