We can discuss it today at 3pm, Alex, if the time works for you. WebEx is fine, but we should make sure to record the session + share follow up notes with the community. The best approach would be - to update the FS with the design changes for this particular part of the code.
Will be waiting for the webex invitation from you. -Alena. On 4/28/14, 11:23 AM, "Alex Ough" <alex.o...@sungardas.com> wrote: >what do have any web conferencing app? >If not, we can use the webex. >Let me know what time is convenient for you. > > >On Mon, Apr 28, 2014 at 1:46 PM, Alena Prokharchyk < >alena.prokharc...@citrix.com> wrote: > >> Alex, that is totally up to you. I'm ready to review/talk any time. >> >> -alena. >> >> On 4/28/14, 10:35 AM, "Alex Ough" <alex.o...@sungardas.com> wrote: >> >> >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+be >> >>tt >> >> 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.j >>>>>>av >> >>>>a >> >> >>f6743ba >> >> >> >> >> >> >>>>>>api/src/org/apache/cloudstack/api/command/admin/region/UpdateRegionCm >>>>>>d. >> >>>>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 >>>>>>-c >> >>>>or >> >> >>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.j >>>>>>av >> >>>>a >> >> >>PRE-CREATION >> >> >> >> >>engine/schema/src/org/apache/cloudstack/multiregion/dao/RsyncDao.java >> >> >>PRE-CREATION >> >> >> >> >> >> >>>>>>engine/schema/src/org/apache/cloudstack/multiregion/dao/RsyncDaoImpl. >>>>>>ja >> >>>>va >> >> >> PRE-CREATION >> >> >> engine/schema/src/org/apache/cloudstack/region/RegionVO.java >> >>608bd2b >> >> >> >> >> >> >>>>>>plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/ >>>>>>ne >> >>>>tw >> >> >>ork/contrail/management/MockAccountManager.java 957f708 >> >> >> plugins/pom.xml 9b391b8 >> >> >> >> >> >> >>>>>>server/resources/META-INF/cloudstack/core/spring-server-core-managers >>>>>>-c >> >>>>on >> >> >>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/ChildTestConfigurat >>>>>>io >> >>>>n. >> >> >>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 >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> >>
