On 4/8/14, 12:03 PM, "Alex Ough" <alex.o...@sungardas.com> wrote:
>I understand that, but I don't understand why their values can't be >assigned through the dao methods when necessary. >But if you still feel uncomfortable, do you think it is ok to include the >job to store the datetime values in another table into the same >transaction >of creating/removing/updating the resources in their manager methods to >guarantee the atomicity? Yes, as long as this transaction is made from your plugin code, not from GenericDao itself. -Alena. > >Thanks >Alex Ough > > >On Tue, Apr 8, 2014 at 2:45 PM, Alena Prokharchyk < >alena.prokharc...@citrix.com> wrote: > >> Alex, >> >> Take for example the Removed field. Its not being managed through none >>of >> the CS interfaces, and you can't set this value neither from the API, >>nor >> from the Orchestration layer. Its being set automatically in GenericDao >> once the object is set for removal by the very same Dao. >> >> The same applies to the Created/Updated fields. >> >> >> -Alena. >> >> On 4/8/14, 11:41 AM, "Alex Ough" <alex.o...@sungardas.com> wrote: >> >> >#2 : I will, and YES, you need to change the modes of all regions, >>which >> >also means that you can sync the resources only in the regions you >>select. >> > >> > >> >#1: Well... it looks like I still need more information why they can't >>be >> >added to their attributes. They are also managed through the their dao >> >interfaces, and the values are assigned only during the full scan to >> >preserve the original value when they >> > are missed. >> > >> > >> >Thanks >> >Alex Ough >> > >> > >> >On Tue, Apr 8, 2014 at 2:29 PM, Alena Prokharchyk >> ><alena.prokharc...@citrix.com> wrote: >> > >> > >> > >> >On 4/8/14, 11:26 AM, "Alex Ough" <alex.o...@sungardas.com> wrote: >> > >> >>Yes, I really hope so from now on :-) >> >> >> >> >> >>For #2, the current setting is NOT syncing the resources and these are >> >>the steps to enable this feature >> >> - Set the "mode" value of the remote regions to sync into 'sync >>on' >> >> - Set the value of global parameter, "region.full.scan.interval" >>(the >> >>default value is '0"), into value of milliseconds to enable the Full >>Scan >> >> Let me know if you want a different way. >> > >> > >> > >> >Sounds good to me. We have to document it though very clearly under >>³how >> >to disable the feature² section. When you set the mode, you have to >>set it >> >for all the regions to completely disable the feature, right? >> > >> >> >> >> >> >>For #1, after another thought, I'm not still clear why those datetime >> >>values can't be added to their attributes. As you recommended, we can >>use >> >>another table to track them, but there can be an issue of partial >>commits >> >>when only the resource are successfully >> >> created/updated/removed and their datetime values fail to be stored >> >>because they are NOT in the same transaction. >> >> >> >> >> >>And we also need to track the 'removed' datetime because when a >>resource >> >>exists in only either the local or the remote region, we don't know >> >>whether it was created but it failed to be created in another region >>or >> >>it was removed but it failed to be removed >> >> in another region. That's why I track the "removed" datetime and >> >>preserve the original datetime. >> > >> > >> >I¹m fine with keeping the removed field, as long as you don¹t update >>this >> >field in CS DB. >> > >> > >> >> >> >> >> >>Let me think about the #1 more closely and let you know what I find. >> >>So please let me know if you have any other suggestion related with >>this. >> >>Thanks >> >>Alex Ough >> >> >> >> >> >> >> >>On Tue, Apr 8, 2014 at 2:09 PM, Alena Prokharchyk >> >><alena.prokharc...@citrix.com> wrote: >> >> >> >>Sorry, Alex, for not providing the input earlier, I understand that >> >>requests like this shouldn¹t come the last minute. But unfortunately >>your >> >>FS didn¹t have enough information to caught these design problems - >>the >> >>fact that the GenericDaoFields modified/removed >> >> get modified directly - from the very start, and only by reviewing >>the >> >>review board ticket you could see it. Hopefully the next time the >> >>experience will be more smooth for both parties. Meanwhile I¹m ready >>to >> >>help with whatever questions you have. >> >> >> >> >> >>Yes, only 2 things need to be fixed. #2 should be pretty >>straightforward. >> >>Make sure your component injects the global config variable if you >>decide >> >>to make a switch via global config. For that, your class has to >>implement >> >>Configurable interface. You can >> >> find examples of how this interface is utilized by existing CS >> >>components. Here is the FS: >> >> >> >> >> >>https://cwiki.apache.org/confluence/display/CLOUDSTACK/Configuration >> >> >> >> >> >>For #1 - I don¹t think you need to keep track of Removed date times. >>Once >> >>the removed field is set in Region1, its safe to just go ahead and >>remove >> >>the entry in all the rest of the regions. You can¹t (and don¹t need >>to) >> >>perform any other operation on the >> >> resource if its removed in one of the regions. >> >>You just have to fix it in created/updated(modified) fields. >> >> >> >> >> >>-Alena. >> >> >> >> >> >>From: Alex Ough <alex.o...@sungardas.com> >> >>Date: Tuesday, April 8, 2014 at 10:39 AM >> >>To: Alena Prokharchyk <alena.prokharc...@citrix.com> >> >>Cc: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Alex >>Huang >> >><alex.hu...@citrix.com> >> >> >> >>Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among >> >>Multiple Regions (Core Changes) >> >> >> >> >> >> >> >> >> >> >> >>Alena, >> >> >> >> >> >>It would be really nice if I had this feedback when I presented this >>at >> >>the beginning. >> >>Anyway, so are only these 2 things necessary to be merged into the >> >>master? >> >> >> >> >> >>1. find another way to store the created/updated/removed datetimes of >>the >> >>resources >> >>2. provide a way to disable this feature. >> >> >> >> >> >>Let me try to resolve those 2 and let you know once done, >> >>so please let me know now if there is anything else needed. >> >> >> >> >> >>Thanks >> >>Alex Ough >> >> >> >> >> >> >> >>On Tue, Apr 8, 2014 at 1:02 PM, Alena Prokharchyk >> >><alena.prokharc...@citrix.com> wrote: >> >> >> >>Alex, thank you for the explanation. But I think there should be >>another >> >>way of saving all the ³updated²/³created² info rather than updating >>the >> >>UserVO/AccountVO/DomainVO fields directly. >> >> >> >> >> >>The one way I can think of - you can create a helper table where you >> >>store the ref UUID->Region->UpdatedTime stamps for all Local/Remote >> >>regions, and you update/compare those fields. Your component is a >> >>business logic component, and it shouldn¹t modify >> >> the fields originally set by GenericDao (Removed/Created/Modified). >>You >> >>should operate only with parameters that are exposed to changes coming >> >>through the APIs. >> >> >> >> >> >>-Alena. >> >> >> >> >> >>From: Alex Ough <alex.o...@sungardas.com> >> >>Date: Monday, April 7, 2014 at 9:16 PM >> >>To: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Alena >> >>Prokharchyk <alena.prokharc...@citrix.com>, >> >> Alex Huang <alex.hu...@citrix.com> >> >>Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among >> >>Multiple Regions (Core Changes) >> >> >> >> >> >> >> >>And all 4 of the recommendations Alex Hwang gave were already >>implemented >> >>to support the real time synchronization (#1), >> >>but like I said in the previous email, we need to support the full >>scan >> >>(#2) to cover any failures during the synchronization. >> >> >> >> >> >>Thanks >> >>Alex Ough >> >> >> >> >> >> >> >> >> >> >> >>On Tue, Apr 8, 2014 at 12:10 AM, Alex Ough >> >><alex.o...@sungardas.com> wrote: >> >> >> >>Alena/Alex, >> >> >> >> >> >>I think I need to give some explanation how this works. >> >> >> >> >> >>There are 2 ways of sync. >> >> >> >> >> >>1. The real time sync : Whenever a resource is create/updated/removed, >> >>each region gets the event of that and requests the same job to all >> >>remote regions using API interfaces, which will create/update/remove >>the >> >>same resource in each remote region in the >> >> real time. >> >> >> >> >> >>2. Full scan : There can be some interruption (whatever the reason is) >> >>that makes fail the real time sync, so each region scans its own >> >>resources with an interval and compares them with those of remote >> >>regions. This is a little tricky because we need to >> >> find out whose change is later when there is any discrepancy between >> >>resources in the local & remote regions and this is why we need >>maintain >> >>the created/updated/removed times. >> >> >> >> >> >> 1) When a resource exists in both local (L) and remote regions >>(R1) >> >> - compare the 'updated' times of both >> >> - Update the local's resource using remote resource attributes >> >>only if the updated time of the remote region is later >> >> - Store the "updated" time of the remote resource (not the >> >>current time) as the local's updated time >> >> - If we store the current time as the updated time, it will >>cause >> >>an mis-sync >> >> because if the resource was updated in another remote region >> >>(R2) a little after it was updated in the remote region(R1), >> >> update in R2 region will be overwritten by the update in R1 >> >>because the current time is later than the updated time of R2 >> >> >> >> >> >> 2) When a resource exists only in the local region >> >> - Find if there was a removal event of this resource in the >>remote >> >>region >> >> - and if so, compare the time when the event occurred with the >> >>created time in the local region >> >> - Remove the resource from the local only if the event time is >> >>later >> >> - Like #1, store the time the event was occurred as the >>'removed' >> >>time of the removed resource in the local region >> >> >> >> >> >> 3) When a resource exists only in the remote region >> >> - Find if the resource in the local has been removed >> >> - If so, compare the removal time in the local and the created >>time >> >>in the remote >> >> - Create the resource in the local only if either the local was >>not >> >>removed or the created time of the remote is later than the removal in >> >>local >> >> - LIke #1 & #2, store the time of the created of the remote >>region >> >>as the 'created' time of the newly created in the local region. >> >> >> >> >> >>I hope this will help you understand how the create/updated/removed >>times >> >>are managed. >> >>If there is no failure in the real time sync (#1), we'll not need the >> >>full scan (#2) and we don't need to worry about the times, >> >>but there is no way to guarantee #1 not to be failed, so #2 is very >> >>important to be supported. >> >> >> >> >> >>Please let me know what you think and we can have a conference call if >> >>you want. >> >>Thanks >> >>Alex Ough >> >> >> >> >> >> >> >>On Mon, Apr 7, 2014 at 8:39 PM, Alena Prokharchyk >> >><alena.prokharc...@citrix.com> wrote: >> >> >> >> >> >>----------------------------------------------------------- >> >>This is an automatically generated e-mail. To reply, visit: >> >> >> >>https://reviews.apache.org/r/20099/#review39753 >> >>----------------------------------------------------------- >> >> >> >> >> >> >> >>Alex, just discussed your changes to existing CS code >> >>(Account/Domain/UserManager, VOs, Daos, GenericDaoBase), with Alex >>Huang. >> >>Here are the concerns: >> >> >> >> >> >>1) there is no need to update the Removed field. Once the entry is >> >>removed, you should purely rely on the removed not to be null, in >>order >> >>to update it in the second region database. >> >> >> >>2) The big issue - you can't trigger generic dao fields modification - >> >>Modified/Removed - through the Management/Services layer. Once the >>field >> >>is defined in Generic Dao Attributes, it should be populated by >>generic >> >>dao only. It should never be populated by >> >> the business logic component what your service is. >> >> >> >>You should remove the field updates from all the methods I've >>mentioned >> >>in my previous review (for example, >> >> >> >> AccountManager >> >> >> >> public boolean disableAccount(long accountId, Date modified) throws >> >>ConcurrentOperationException, ResourceUnavailableException;) >> >> >> >>and find another way of keeping track of modified attribute update for >> >>the CS objects. >> >> >> >>Alex suggested a following solution: >> >>========================================================= >> >>* Every time user/domain/account is created/updated/removed in CS, >> >>certain Action event is generated. There is a way to pubish this >>event to >> >>the event bus, either rabbitMQ or in-memory bus. >> >>* Instead of scanning user/domain/account tables, your plugin should >> >>listen to the create/update/remove user/domain/account events >>published >> >>to the event bus, and update the user/domain/account in all regions in >> >>the system accordingly >> >>* Of course, you have to skip the event processing for the event >> >>generated by your plugin - this part can be tricky >> >>* The events processing should be synchronized on the resource type >> >>(user/account/domain) + event timestamp in your plugin, so all the >>events >> >>for the same resource are processed in the order based on the >>timeStamp >> >>they were generated >> >> >> >>When to use in-memory bus vs RabbitMQ? It depends on how your service >> >>runs. If it runs on each management server, then it would be >>responsible >> >>only for events generated by this management server, and should use >> >>events published to in-memory bus. >> >>If your server runs only on one of the Management servers, and there >>are >> >>multiple servers in the cluster, you should listen to all the events >>from >> >>all the management servers in cluster. In this case, you should use >> >>RabbitMQ bus. >> >> >> >>Here is the link to the in-memory bus FS: >> >> >> >>https://cwiki.apache.org/confluence/display/CLOUDSTACK/In-memory+event+bu >> >>s >> >>But I'm sure you are familiar how this stuff works as you utilize the >> >>event bus in your code already. >> >> >> >>Or you can find another solution; but this solution should never >>involve >> >>direct modification for "Modified" field by your plugin. >> >> >> >>3) You should add a way to disable your plugin through Global >>Config/API. >> >>This should be done to support the case when user/domain/account >>entries >> >>are generated by some app running on top of CS, for all the regions in >> >>the system. In this case admin controls >> >> the user/domain/account entries outside of the CS, and synchronize >>them >> >>outside as well - so might need to disable your plugin. >> >> >> >>- Alena Prokharchyk >> >> >> >> >> >>On April 7, 2014, 7:13 p.m., Alex Ough wrote: >> >>> >> >>> ----------------------------------------------------------- >> >>> This is an automatically generated e-mail. To reply, visit: >> >>> https://reviews.apache.org/r/20099/ >> >> >> >>> ----------------------------------------------------------- >> >>> >> >>> (Updated April 7, 2014, 7:13 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/domain/Domain.java 365a705 >> >>> api/src/com/cloud/event/EventTypes.java 39ef710 >> >>> api/src/com/cloud/user/Account.java b912e51 >> >>> api/src/com/cloud/user/AccountService.java 7e37b38 >> >>> api/src/com/cloud/user/User.java 36e9028 >> >>> api/src/com/cloud/user/UserAccount.java c5a0637 >> >>> 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.ja >>>>>va >> >>>f6743ba >> >> >> >>> >> >>>>>api/src/org/apache/cloudstack/api/command/admin/region/UpdateRegionCmd >>>>>.j >> >>>a >> >>>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- >>>>>co >> >>>r >> >>>e-daos-context.xml 489b37d >> >>> engine/schema/src/com/cloud/domain/DomainVO.java f6494b3 >> >>> engine/schema/src/com/cloud/user/AccountVO.java 0f5a044 >> >>> engine/schema/src/com/cloud/user/UserAccountVO.java cef9239 >> >>> engine/schema/src/com/cloud/user/UserVO.java 68879f6 >> >>> engine/schema/src/org/apache/cloudstack/region/RegionVO.java >>608bd2b >> >>> framework/db/src/com/cloud/utils/db/Attribute.java 82c2bdb >> >>> framework/db/src/com/cloud/utils/db/GenericDao.java cb401cd >> >>> framework/db/src/com/cloud/utils/db/GenericDaoBase.java 2052aad >> >>> framework/db/src/com/cloud/utils/db/SqlGenerator.java befe34b >> >>> framework/db/test/com/cloud/utils/db/GenericDaoBaseTest.java >>aef0c69 >> >>> framework/db/test/com/cloud/utils/db/SqlGeneratorTest.java >> >>>PRE-CREATION >> >>> >> > >> > >> >>>>>plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/n >>>>>et >> >>>w >> >>>ork/contrail/management/MockAccountManager.java 957f708 >> >>> plugins/pom.xml 9b391b8 >> >> >> >>> >> >>>>>server/resources/META-INF/cloudstack/core/spring-server-core-managers- >>>>>co >> >>>n >> > >> >>>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/api/query/vo/AccountJoinVO.java 8d642ed >> >>> server/src/com/cloud/api/query/vo/UserAccountJoinVO.java ed29284 >> >>> server/src/com/cloud/event/ActionEventUtils.java 28e5680 >> >>> 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/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 >> >>> >> >>> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> >>
