----------------------------------------------------------- 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+bus 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.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/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/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/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 > >
