For the account sync, we can make use of the new event framework: instead of calling account sync inline with account creation, simply emit an event. The event can be consumed by the account sync service.
On 1/29/13 11:06 PM, "Kishan Kavala" <kishan.kav...@citrix.com> wrote: >Chiradeep, > Please find my comments inline. > >> -----Original Message----- >> From: Chiradeep Vittal [mailto:chiradeep.vit...@citrix.com] >> Sent: Wednesday, 30 January 2013 1:42 AM >> To: cloudstack-dev@incubator.apache.org >> Subject: Re: [MERGE] Regions branch to master >> >> 0. Doesn't compile (commit d3089ba2a5684) >[KK] Sorry about that. Few new files remained in my local repo. I missed >adding them. They are added now. > >> 1. Why does the createRegion >> API take an integer id? I think I know the answer, but it could be >>documented >> in the FS / code >[KK] Number of regions is expected to be small and won't go beyond the >integer range. Id is also not auto-incremented. Documented in FS. > >>2. Why doesn't the createRegion API check for duplicate >> region names? >[KK] Yes region names also should be unique. I'll add checks for the same > >> 3. I'd argue that end users are more familiar with region names than >>ids. >[KK] Right. Even on the UI, users will see names more than Ids. > >> 4. Describe API and secret key in greater detail in the FS and >>annotation. >> Why is it needed? >[KK] Admin user api_key/secret_key are needed to make sync API calls to >peer regions. These are added during addRegion API call. Updated FS with >the details. > >> 5. I think we should move away from implementing the service api and the >> provisioning/orchestration api in the same file. So, split into >> RegionServiceImpl and RegionManagerImpl >[KK] Sure, I'll make this change and split into ServiceImpl and >ManagerImpl. > > 6. Do we really need propagate* >> APIs? (note spelling mistake in actual API). This is going to be so >>error prone >> and full of corner cases that will require manual fiddling to fix >>anyway. Can >> we assume a 3rd party service that will perform the sync? >[KK] I'll correct the spelling propogate -> propagate. Agree that there >are cases when API can fail. Failures are logged in region_sync table >which can be processed offline. >Are you suggesting that we should assume that a 3rd party service which >will perform DB sync instead of API sync? For APIs like deletAccount, DB >sync will skip resource cleanup in some cases. >I can make API sync optional for now. > >> 7. Please add more comments (schema, interface, api) >[KK] I'll add more comments. > >8. The new package >> naming scheme seems to be org.apache.cloudstack, so could we use that? >[KK] I'll change the package naming to org.apache.cloudstack > >> >> >> >> On 1/29/13 6:47 AM, "Chip Childers" <chip.child...@sungard.com> wrote: >> >> >On Tue, Jan 29, 2013 at 4:45 AM, Kishan Kavala >> ><kishan.kav...@citrix.com> >> >wrote: >> >> I would like to merge Regions feature to master >> >> >> >> Spec: >> >> >> >>https://cwiki.apache.org/confluence/display/CLOUDSTACK/AWS- >> Style+Regio >> >>ns+ >> >>Functional+Spec >> >> >> >> Jira ticket: >> >> https://issues.apache.org/jira/browse/CLOUDSTACK-241 >> >> >> >> Branch: >> >> regions (latest master code is merged to regions branch) >> >> >> >> Notes: >> >> 1. If an environment has only 1 region, functionality will be same as >> >>the current CS installation, no impact. >> >> 2. In multi-region setup, any failure in propagating >> >>account/user/domain changes to other regions will be logged in >> >>region_sync table. These changes have to be propagated manually. >> >> 3. Branch is updated with latest master and all unit tests passed >> >> >> >> Regards, >> >> Kishan >> > >> >Kishan, >> > >> >+1 from me! We probably want someone else to comment as well, but I'm >> >really glad to see that the tests were extended to account for the >> >region entity. >> > >> >I also appreciate that the addition of the region ID param for the API >> >calls remained optional. >> > >> >Thanks! >
