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!
