Alena, It has been reduced almost twice because a lot has been separated from the CS and moved to the plug-in not because they are 'unnecessary'. Please remember that my initial implementation was inside the CS not as a plug-in as I said in the previous email.
Of course, I asked and urged the review repeatedly and you'll see the all the histories of them if you find emails using this subject, which started 10/17/13. [DISCUSS] Domain-Account-User Sync Up Among Multiple Regions Even if I asked so many times, unfortunately, I couldn't get an actual feedback until Daan finally asked Chiradeep and you to review them, which is 3/10/14. Kishan, I posted 2 questions, so please guide me for the questions. Thanks Alex Ough On Thu, Jun 26, 2014 at 12:57 PM, Alena Prokharchyk < alena.prokharc...@citrix.com> wrote: > Alex, > > By “huge” I’ve meant that there was a lot of repetitive hardcoded > things, lot of unnecessary changes to the CS orchestration layer. If you > compare a number of changes now and originally, you can see that it reduced > almost twice. > > But lets discuss the complains about lack of initial review as its more > important question. > > Review of the design spec should happen before you start > designing/coding. As I jumped on review much later, after you’ve submitted > the entire plugin code, so I I didn’t participate in “Feature Request” > discussion review that might have happened earlier. And I do assume that > the reviews/emails exchanges were done at that initial phase? You should > have contacted the people participating in the initial phase, and ask them > for the review as well. > > As a part of my review, I’ve made sure to cover the things I’m certain > should have been changed. I’ve reviewed the feature logic as well, > consulting the FS you’ve written. I’m not saying that there is anything > wrong with your initial design, but asking for a second opinion from the > guys who have more expertise in Regions. > > Kishan, please help to do the final review the Alex’s plugin design > https://reviews.apache.org/r/17790 > > Thank you, > Alena. > From: Alex Ough <alex.o...@sungardas.com> > Date: Wednesday, June 25, 2014 at 9:03 PM > > To: Alena Prokharchyk <alena.prokharc...@citrix.com> > Cc: Kishan Kavala <kishan.kav...@citrix.com>, "dev@cloudstack.apache.org" > <dev@cloudstack.apache.org>, Murali Reddy <murali.re...@citrix.com>, Ram > Ganesh <ram.gan...@citrix.com>, Animesh Chaturvedi < > animesh.chaturv...@citrix.com> > Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among > Multiple Regions (Core Changes) > > Alena, > > I understand that you have been helping a lot to make my codes to match > the coding standards, but I'm not sure what you mean by "the code base was > unnecessary huge". > The initial implementation was to support the synchronization inside the > CS because this feature is missing in the current multiple region support, > and most of jobs were to separate the implementation from the CS because > you guys wanted me to provide it as a plugin. > > And I kept asking reviews for the design spec from when I published the > documents with initial prototype, it took a while for you to start to > review my implementation and they have been mostly about the coding > standards instead of the logic itself. So I'm saying that it would have > been better if there has been someone to review the design spec and the > prototype from the initial phase. > > Again, I really appreciate your help to come this far, but it was also > very painful for me. > Thanks > Alex Ough > > > On Wed, Jun 25, 2014 at 10:41 PM, Alena Prokharchyk < > alena.prokharc...@citrix.com> wrote: > >> Alex, >> >> In the beginning the code was not very well organazied, didn't match >> coding standarts (no use of spring, misleading names, not segregated to its >> own plugin), and the code base was unneccessary huge. >> All of the above it very hard to review and understand the code logic >> from the beginning and engage more people to the review. Therefore >> Chiradeep pointed it in his original review that the code needs to match CS >> standarts first, and be better organized. I helped to review the fixes, and >> did logic review as well after the code came into “reviewable” shape. >> >> I'm asking Kishan/Murali to look at it to see if anything is missing or >> incorrect in the final review, not to make you override or change >> everything you've already put in. >> >> Thank you, >> Alena. >> >> From: Alex Ough <alex.o...@sungardas.com> >> Date: Wednesday, June 25, 2014 at 7:12 PM >> >> To: Alena Prokharchyk <alena.prokharc...@citrix.com> >> Cc: Kishan Kavala <kishan.kav...@citrix.com>, "dev@cloudstack.apache.org" >> <dev@cloudstack.apache.org>, Murali Reddy <murali.re...@citrix.com>, Ram >> Ganesh <ram.gan...@citrix.com>, Animesh Chaturvedi < >> animesh.chaturv...@citrix.com> >> Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among >> Multiple Regions (Core Changes) >> >> Alena, >> >> Don't get me wrong. What I'm saying is that it would have been better >> if you asked the review to whomever you thought was important when you >> started the review. >> >> Thanks >> Alex Ough >> >> >> On Wed, Jun 25, 2014 at 9:45 PM, Alena Prokharchyk < >> alena.prokharc...@citrix.com> wrote: >> >>> Alex, >>> >>> I did my best to review the code, made sure it came in shape with the >>> CS guidelines and java code style There was no way to anticipate all the >>> things to fix originally, as every subsequent review update added more >>> things to fix as the review code was new/refactored. >>> >>> And I don’t see anything wrong about asking for a FINAL opinion from >>> other people on the mailing list, considering some of them participated in >>> the review process along the way already (Kishan). Anybody can review the >>> review ticket till its closed, and point to the items that other reviewers >>> might have missed. >>> >>> Thank you, >>> Alena. >>> >>> From: Alex Ough <alex.o...@sungardas.com> >>> Date: Wednesday, June 25, 2014 at 6:33 PM >>> To: Alena Prokharchyk <alena.prokharc...@citrix.com> >>> Cc: Kishan Kavala <kishan.kav...@citrix.com>, "dev@cloudstack.apache.org" >>> <dev@cloudstack.apache.org>, Murali Reddy <murali.re...@citrix.com>, >>> Ram Ganesh <ram.gan...@citrix.com>, Animesh Chaturvedi < >>> animesh.chaturv...@citrix.com> >>> >>> Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among >>> Multiple Regions (Core Changes) >>> >>> Thanks Alena, and I'm glad if they spend time for the review, but >>> could it be a little earlier for you to ask them to review instead of at >>> the last moment? >>> I'm really exhausted with repeatedly added items whenever I post a >>> review. >>> >>> Thanks >>> Alex Ough >>> >>> >>> On Wed, Jun 25, 2014 at 7:44 PM, Alena Prokharchyk < >>> alena.prokharc...@citrix.com> wrote: >>> >>>> Alex, looks fine to me. Make sure that you put the regionId >>>> validation as our in-built API validation won’t work in this case because >>>> there is no UUID field support for the Region object. You can check how >>>> validation is begin done in updateRegion/deleteRegion scenarios. >>>> >>>> Kishan/Murali, can you please spend some time doing the final review >>>> for Alex’s tickets? As you are the original developers for Region, and >>>> probably have the most expertise on the topic. I don’t want to commit the >>>> fixes before I hear “ship it” from both of you, guys. >>>> >>>> Thanks, >>>> Alena. >>>> From: Alex Ough <alex.o...@sungardas.com> >>>> Date: Wednesday, June 25, 2014 at 4:02 PM >>>> To: Kishan Kavala <kishan.kav...@citrix.com> >>>> Cc: Alena Prokharchyk <alena.prokharc...@citrix.com>, " >>>> dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Murali Reddy < >>>> murali.re...@citrix.com>, Ram Ganesh <ram.gan...@citrix.com>, Animesh >>>> Chaturvedi <animesh.chaturv...@citrix.com> >>>> >>>> Subject: Re: Review Request 20099: Domain-Account-User Sync Up Among >>>> Multiple Regions (Core Changes) >>>> >>>> Hi Alena, >>>> >>>> Can you confirm if this fix is correct? >>>> >>>> @Parameter(name = ApiConstants.ORIGINATED_REGION_ID, type = >>>> CommandType.INTEGER, description = "Region where this account is created.", >>>> since = "4.5") >>>> private Integer originatedRegionId; >>>> >>>> Thanks >>>> Alex Ough >>>> >>>> >>>> On Wed, Jun 25, 2014 at 11:03 AM, Kishan Kavala < >>>> kishan.kav...@citrix.com> wrote: >>>> >>>>> Alex, >>>>> >>>>> You can refer to the code from initDataSource method in >>>>> Transaction.java. >>>>> >>>>> Properties file can be loaded using the following: >>>>> >>>>> >>>>> >>>>> *File dbPropsFile = PropertiesUtil.findConfigFile(propsFileName);* >>>>> >>>>> >>>>> >>>>> *From:* Alex Ough [mailto:alex.o...@sungardas.com] >>>>> *Sent:* Wednesday, 25 June 2014 4:31 PM >>>>> *To:* Kishan Kavala >>>>> *Cc:* Alena Prokharchyk; dev@cloudstack.apache.org; Murali Reddy; Ram >>>>> Ganesh; Animesh Chaturvedi >>>>> >>>>> *Subject:* Re: Review Request 20099: Domain-Account-User Sync Up >>>>> Among Multiple Regions (Core Changes) >>>>> >>>>> >>>>> >>>>> Thanks Kishan, but there seems to be lots of 'db.properties' files, so >>>>> which one should be referenced? >>>>> >>>>> >>>>> >>>>> Alex Ough >>>>> >>>>> >>>>> >>>>> On Wed, Jun 25, 2014 at 2:25 AM, Kishan Kavala < >>>>> kishan.kav...@citrix.com> wrote: >>>>> >>>>> Alex, >>>>> >>>>> As Alena mentioned, it is admin’s responsibility to keep ids same >>>>> across Regions. Ids should be used as unique identifier. Region name is >>>>> merely descriptive name and its mostly associated with geographic >>>>> location. >>>>> >>>>> Also note that region name can be updated anytime using updateRegion >>>>> API. >>>>> >>>>> >>>>> >>>>> Unlike, other internal Ids in CS, region Ids are assigned by admin. So >>>>> exposing region Id to admin should not be an issue. >>>>> >>>>> >>>>> >>>>> Id of the local region cannot be guaranteed to be “1” always. Region >>>>> Id has to be unique across all regions. While creating new region admin >>>>> will provide unique region id to *cloud-setup-databases* script. Id >>>>> of the local region is stored in db.properties. To identify a Local region >>>>> you can use one of the following options: >>>>> >>>>> 1. Look up region.id in db.properties >>>>> >>>>> 2. Add a new column in region table >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> *From:* Alex Ough [mailto:alex.o...@sungardas.com] >>>>> *Sent:* Wednesday, 25 June 2014 8:18 AM >>>>> *To:* Alena Prokharchyk >>>>> *Cc:* dev@cloudstack.apache.org; Kishan Kavala; Murali Reddy; Ram >>>>> Ganesh; Animesh Chaturvedi >>>>> >>>>> >>>>> *Subject:* Re: Review Request 20099: Domain-Account-User Sync Up >>>>> Among Multiple Regions (Core Changes) >>>>> >>>>> >>>>> >>>>> There is one thing that was not mentioned, which is that currently the >>>>> id of 'Local' region is always 1 and if we do not guarantee that, there is >>>>> no way to find out which is the local region unless we add one more field >>>>> to tells which is the local region. >>>>> >>>>> I'm wondering if we have a solution for this now. >>>>> >>>>> >>>>> >>>>> Thanks >>>>> >>>>> Alex Ough >>>>> >>>>> >>>>> >>>>> On Tue, Jun 24, 2014 at 9:59 PM, Alex Ough <alex.o...@sungardas.com> >>>>> wrote: >>>>> >>>>> I agree with that the ids are unique identifier, but they are usually >>>>> internal purpose not exposed to the users. So it is a little strange to >>>>> ask >>>>> users to assign ids when they add new regions. And if we do not allow >>>>> duplicated names, I'm not sure why it is not good to use names as a unique >>>>> identifier. >>>>> >>>>> >>>>> >>>>> It's been a long way to come this far with several reasons, so I >>>>> really want to wrap this up as soon as possible, and this doesn't seem to >>>>> be a major obstacle, so let me just use 'id' as a parameter if there is no >>>>> one with a different thought until tomorrow morning. >>>>> >>>>> >>>>> >>>>> Thanks >>>>> >>>>> Alex Ough >>>>> >>>>> >>>>> >>>>> On Tue, Jun 24, 2014 at 8:52 PM, Alena Prokharchyk < >>>>> alena.prokharc...@citrix.com> wrote: >>>>> >>>>> Alex, id is used as a unique identifier for CS objects. And it is the >>>>> CS requirement to refer to the object by id if the id is present. Look at >>>>> all the other APIs. We nowhere refer to the network/vpc/vm by name just >>>>> because its more human readable. The id is used by Api layer when >>>>> parameter >>>>> validation is done, by lots of Dao methods (findById is one of them), etc. >>>>> Even look at updateRegion/deleteRegion – we don’t refer to them by name, >>>>> but by the id. >>>>> >>>>> >>>>> >>>>> The reason why Kishan added the support for controlling the id by >>>>> adding it to the createRegion call (and making it unique) is exactly that >>>>> – >>>>> region administrator can decide what id to set on the region, and to >>>>> introduce the region with the same id to the other regions’ db. >>>>> >>>>> >>>>> >>>>> So I would still suggest on using the id of the region in the API >>>>> calls you are modifying. Unless developers who worked on regions feature – >>>>> Kishan/Murali – come up with the valid objection. >>>>> >>>>> >>>>> >>>>> Thanks, >>>>> >>>>> Alena. >>>>> >>>>> >>>>> >>>>> *From: *Alex Ough <alex.o...@sungardas.com> >>>>> *Date: *Tuesday, June 24, 2014 at 5:41 PM >>>>> >>>>> >>>>> *To: *Alena Prokharchyk <alena.prokharc...@citrix.com> >>>>> *Cc: *"dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Kishan >>>>> Kavala <kishan.kav...@citrix.com>, Murali Reddy < >>>>> murali.re...@citrix.com>, Ram Ganesh <ram.gan...@citrix.com>, Animesh >>>>> Chaturvedi <animesh.chaturv...@citrix.com> >>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up >>>>> Among Multiple Regions (Core Changes) >>>>> >>>>> >>>>> >>>>> We can use the same ids & names, but we don't have to use the same ids >>>>> if we use names, which is a little easier because names are user readable >>>>> but ids are not, so we don't need to memorize/check all the ids when we >>>>> add >>>>> new regions in multiple regions, which can be confusing. >>>>> >>>>> >>>>> >>>>> Thanks >>>>> >>>>> Alex Ough >>>>> >>>>> >>>>> >>>>> On Tue, Jun 24, 2014 at 8:33 PM, Alena Prokharchyk < >>>>> alena.prokharc...@citrix.com> wrote: >>>>> >>>>> Aren’t we supposed to sync the regions across the multiple regions >>>>> Dbs? Because that’s what region FS states: >>>>> >>>>> >>>>> >>>>> >>>>> https://cwiki.apache.org/confluence/display/CLOUDSTACK/AWS-Style+Regions+Functional+Spec, >>>>> “Adding 2nd region” paragraph, bullet #4: >>>>> >>>>> >>>>> >>>>> 1. Install a 2nd CS instance. >>>>> >>>>> 2. While installing database set region_id using -r option in >>>>> cloud-setup-databases script (Make sure *database_key* is same across >>>>> all regions). >>>>> >>>>> *cloud-setup-databases cloud:**<**dbpassword**>**@localhost >>>>> --deploy-as=root:**<**password**>** -e **<**encryption_type**>** -m * >>>>> *<**management_server_key**>** -k **<**database_key**> -r <region_id>* >>>>> >>>>> 3. Start mgmt server >>>>> >>>>> 4. *Using addRegion API, add region 1 to region 2 and also region 2 >>>>> to region 1.* >>>>> >>>>> >>>>> >>>>> I assume that we expect the admin to add the region with the same name >>>>> and the same id to ALL regions Dbs (both id and name should be passed to >>>>> createRegion call). So they are all in sync. Isn’t it the requirement? If >>>>> so, we should rely on the fact that all regions Dbs will have the same set >>>>> of regions having the same ids and names cross regions. >>>>> >>>>> >>>>> >>>>> Thanks, >>>>> >>>>> Alena. >>>>> >>>>> *From: *Alex Ough <alex.o...@sungardas.com> >>>>> *Date: *Tuesday, June 24, 2014 at 5:17 PM >>>>> *To: *Alena Prokharchyk <alena.prokharc...@citrix.com> >>>>> *Cc: *"dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Kishan >>>>> Kavala <kishan.kav...@citrix.com>, Murali Reddy < >>>>> murali.re...@citrix.com>, Ram Ganesh <ram.gan...@citrix.com>, Animesh >>>>> Chaturvedi <animesh.chaturv...@citrix.com> >>>>> >>>>> >>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up >>>>> Among Multiple Regions (Core Changes) >>>>> >>>>> >>>>> >>>>> What I'm trying to say is that when we pass the ids of regions, the >>>>> receivers do not know what the originated region is and the id of each >>>>> region is not same across all the regions. >>>>> >>>>> >>>>> >>>>> Thanks >>>>> >>>>> Alex Ough >>>>> >>>>> >>>>> >>>>> On Tue, Jun 24, 2014 at 7:35 PM, Alena Prokharchyk < >>>>> alena.prokharc...@citrix.com> wrote: >>>>> >>>>> Alex, thank you for summarizing. >>>>> >>>>> >>>>> >>>>> I still don’t see why id can’t be unique across regions as you can >>>>> control the id assignment – id is required when createRegion call is made. >>>>> And that’s how the region should be represented in other region’s Dbs – by >>>>> its id that is unique across the regions. Kishan/Murali, please confirm. >>>>> >>>>> >>>>> >>>>> Thank you, >>>>> >>>>> Alena. >>>>> >>>>> >>>>> >>>>> *From: *Alex Ough <alex.o...@sungardas.com> >>>>> *Date: *Tuesday, June 24, 2014 at 4:22 PM >>>>> *To: *"dev@cloudstack.apache.org" <dev@cloudstack.apache.org> >>>>> *Cc: *Alena Prokharchyk <alena.prokharc...@citrix.com>, Kishan Kavala >>>>> <kishan.kav...@citrix.com>, Murali Reddy <murali.re...@citrix.com>, >>>>> Ram Ganesh <ram.gan...@citrix.com>, Animesh Chaturvedi < >>>>> animesh.chaturv...@citrix.com> >>>>> >>>>> >>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up >>>>> Among Multiple Regions (Core Changes) >>>>> >>>>> >>>>> >>>>> All, >>>>> >>>>> >>>>> >>>>> There is one open question in this topic, which is to figure out which >>>>> value is appropriate to pass the region object, id or name? >>>>> >>>>> During this implementation, we decided to add the information of >>>>> regions where user/account/domain objects have been originally >>>>> created/modified/removed. >>>>> >>>>> But the ids of regions are not same across the regions and currently >>>>> the regions do not have uuids(they will not be same either if we add them >>>>> to regions), so I'd like to use names. >>>>> >>>>> >>>>> >>>>> Please let me know what you think. >>>>> >>>>> Thanks >>>>> >>>>> Alex Ough >>>>> >>>>> >>>>> >>>>> On Tue, Jun 24, 2014 at 7:05 PM, Animesh Chaturvedi < >>>>> animesh.chaturv...@citrix.com> wrote: >>>>> >>>>> Let’s have the discussion on dev mailing list >>>>> >>>>> >>>>> >>>>> Thanks >>>>> >>>>> Animesh >>>>> >>>>> >>>>> >>>>> *From:* Alena Prokharchyk >>>>> *Sent:* Tuesday, June 24, 2014 3:06 PM >>>>> *To:* Alex Ough; Kishan Kavala; Murali Reddy >>>>> *Cc:* Animesh Chaturvedi; Ram Ganesh >>>>> >>>>> >>>>> *Subject:* Re: Review Request 20099: Domain-Account-User Sync Up >>>>> Among Multiple Regions (Core Changes) >>>>> >>>>> >>>>> >>>>> Adding Kishan to the thread as he was the one who implemented the >>>>> region feature originally. >>>>> >>>>> >>>>> >>>>> Kishan, in a situation when there are 2 regions in the system, we >>>>> expect “region” table to be populated with the same id/name in both Dbs >>>>> for >>>>> both regions, right? So my question is – what uniquely identifies the >>>>> region in CS system in cross region setup – id/name? >>>>> >>>>> >>>>> >>>>> That unique identifier should be the value that is passed to the calls >>>>> you modify, Alex. WE can’t just pass some random name to the call without >>>>> making any further verification. >>>>> >>>>> >>>>> >>>>> Kishan/Murali, please help to verify this part of Alex’s fix as it >>>>> should really be someone with an expertise in Regions. I’ve reviewed the >>>>> rest of the feature, just this one item is open. See my latest comment to >>>>> the https://reviews.apache.org/r/17790/diff/?page=1#0 as well as >>>>> refer to this email thread for the context. >>>>> >>>>> >>>>> >>>>> -Alena. >>>>> >>>>> >>>>> >>>>> *From: *Alena Prokharchyk <alena.prokharc...@citrix.com> >>>>> *Date: *Tuesday, June 24, 2014 at 2:54 PM >>>>> *To: *Alex Ough <alex.o...@sungardas.com> >>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up >>>>> Among Multiple Regions (Core Changes) >>>>> >>>>> >>>>> >>>>> That what would everybody assume 100% just by looking at the parameter >>>>> description and parameter – that you refer to region UUID : "Region where >>>>> this account is created.”/ORIGINATEDREGIONUUID >>>>> >>>>> In CS the UUID has a special meaning. It has to have the UUID format, >>>>> and its randomly generated value that is stored in the DB along with the >>>>> actual db id. I can see that regionVO lacks UUID field. Looks like >>>>> existing >>>>> RegionVO object lacks this filed unlike other CS objects (uservm, etc). I >>>>> will follow up with Murali on that. >>>>> >>>>> >>>>> >>>>> Alex, so originatedRegionUUID refers to the region name, correct?. Why >>>>> don’t use the region id instead? That’s what we do when refer to CS >>>>> objects >>>>> – we always refer to them by id which is unique. Which is true even for >>>>> the >>>>> region: >>>>> >>>>> >>>>> >>>>> mysql> show create table region; >>>>> >>>>> >>>>> >>>>> UNIQUE KEY `id` (`id`), >>>>> >>>>> UNIQUE KEY `name` (`name`) >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> That’s what you do when you manipulate the region itself >>>>> (delete/updateRegion) - refer to the region by its id. And this field is >>>>> returned to you when you call listRegions API: >>>>> >>>>> >>>>> >>>>> http://localhost:8096/?command=listRegions >>>>> >>>>> <region> >>>>> >>>>> <id>1</id> >>>>> >>>>> <name>Local</name> >>>>> >>>>> <endpoint>http://localhost:8080/client/</endpoint> >>>>> >>>>> <gslbserviceenabled>true</gslbserviceenabled> >>>>> >>>>> <portableipserviceenabled>false</portableipserviceenabled> >>>>> >>>>> </region> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> Please correct if I miss something. >>>>> >>>>> -Alena. >>>>> >>>>> >>>>> >>>>> *From: *Alex Ough <alex.o...@sungardas.com> >>>>> *Date: *Tuesday, June 24, 2014 at 2:33 PM >>>>> *To: *Alena Prokharchyk <alena.prokharc...@citrix.com> >>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up >>>>> Among Multiple Regions (Core Changes) >>>>> >>>>> >>>>> >>>>> Thanks for the clarification, but here is a thing. >>>>> >>>>> >>>>> >>>>> I'm passing names as the values of originatedRegionUuids because the >>>>> uuids are randomly generated and the same regions do NOT have the same >>>>> uuidss. >>>>> >>>>> So I'd like to change the parameter types into String. >>>>> >>>>> Let me know if you think otherwise. >>>>> >>>>> >>>>> >>>>> Thanks >>>>> >>>>> Alex Ough >>>>> >>>>> >>>>> >>>>> On Tue, Jun 24, 2014 at 5:17 PM, Alena Prokharchyk < >>>>> alena.prokharc...@citrix.com> wrote: >>>>> >>>>> Alex, >>>>> >>>>> >>>>> >>>>> take a look at ParamProcessWorker class, and how API parameters are >>>>> being dispatched/verified. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> 1) public void processParameters(final BaseCmd cmd, final Map params) >>>>> method >>>>> >>>>> >>>>> >>>>> First of all, EntityType parameter should be defined in the @Parameter >>>>> annotation for the originatedRegionID field. This parameter is used by >>>>> paramProcessWorker to make "if entity exists" validation >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> 2) Check another method in the same class: >>>>> >>>>> >>>>> >>>>> private void setFieldValue(final Field field, final BaseCmd cmdObj, >>>>> final Object paramObj, final Parameter annotation) throws >>>>> IllegalArgumentException, ParseException { >>>>> >>>>> >>>>> >>>>> Thats the method responsible for dispatching/setting the field values. >>>>> Here is the snippet of the code for the case when UUID is defined: >>>>> >>>>> >>>>> >>>>> case UUID: >>>>> >>>>> if (paramObj.toString().isEmpty()) >>>>> >>>>> break; >>>>> >>>>> final Long internalId = >>>>> translateUuidToInternalId(paramObj.toString(), annotation); >>>>> >>>>> field.set(cmdObj, internalId); >>>>> >>>>> break; >>>>> >>>>> >>>>> >>>>> it always transforms the UUID to Long id, not string. And at the end, >>>>> it will be internal DB UUID, not the UUID. If you need the UUID, you have >>>>> to get it by a) retrieving the object from the DB by id b) Getting its >>>>> UUID >>>>> property. >>>>> >>>>> >>>>> >>>>> If you leave it as a String, you will hit IllegalArgumentException at >>>>> "field.set(cmdObj, internalId);" line. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> Hope it answers your questions, and let me know if anything else needs >>>>> to be clarified. >>>>> >>>>> >>>>> >>>>> -alena. >>>>> >>>>> >>>>> >>>>> *From: *Alex Ough <alex.o...@sungardas.com> >>>>> *Date: *Tuesday, June 24, 2014 at 1:57 PM >>>>> >>>>> >>>>> *To: *Alena Prokharchyk <alena.prokharc...@citrix.com> >>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up >>>>> Among Multiple Regions (Core Changes) >>>>> >>>>> >>>>> >>>>> Why do you want to change UUID to 'Long'? >>>>> >>>>> Can you just correct what I fixed? >>>>> >>>>> >>>>> >>>>> On Tue, Jun 24, 2014 at 4:21 PM, Alena Prokharchyk < >>>>> alena.prokharc...@citrix.com> wrote: >>>>> >>>>> You need to put: >>>>> >>>>> >>>>> >>>>> * the entityType parameter to the annotation. >>>>> >>>>> - Change the type to Long as I’ve already mentioned. Check how >>>>> other commands handle the parameters (networkId, vpcId, etc) >>>>> >>>>> —Alena. >>>>> >>>>> >>>>> >>>>> *From: *Alex Ough <alex.o...@sungardas.com> >>>>> *Date: *Tuesday, June 24, 2014 at 12:47 PM >>>>> >>>>> >>>>> *To: *Alena Prokharchyk <alena.prokharc...@citrix.com> >>>>> *Subject: *Re: Review Request 20099: Domain-Account-User Sync Up >>>>> Among Multiple Regions (Core Changes) >>>>> >>>>> >>>>> >>>>> Will this change work? >>>>> >>>>> >>>>> >>>>> @Parameter(name = ApiConstants.ORIGINATED_REGION_ID, type = >>>>> CommandType.UUID, description = "Region UUID where this account is >>>>> created.", since = "4.5") >>>>> >>>>> private String originatedRegionUUID; >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> On Tue, Jun 24, 2014 at 3:25 PM, Alex Ough <alex.o...@sungardas.com> >>>>> wrote: >>>>> >>>>> Alena, >>>>> >>>>> >>>>> >>>>> This is what really frustrates me, but can you give the final items >>>>> instead of keeping adding more items whenever I post a review, please? >>>>> >>>>> Can you gurantee that this is the only item you want me to fix? >>>>> >>>>> >>>>> >>>>> On Tue, Jun 24, 2014 at 3:04 PM, Alena Prokharchyk < >>>>> alena.prokharc...@citrix.com> wrote: >>>>> >>>>> Alex, as a part of the fix, also change the param name to be regionId >>>>> (there should be a value in apiconstants already) as the parameter really >>>>> reflects CS region object, and we usually refer to those as networkID, >>>>> vpcID (not uuid) although uuid are passed in. Check if the rest of the api >>>>> changes you've done, respect this rule. Sorry, out of the office now and >>>>> cant check myself if there are any. >>>>> >>>>> -alena >>>>> >>>>> >>>>> > On Jun 24, 2014, at 11:12 AM, "Alena Prokharchyk" < >>>>> alena.prokharc...@citrix.com> wrote: >>>>> > >>>>> > >>>>> > ----------------------------------------------------------- >>>>> >>>>> > This is an automatically generated e-mail. To reply, visit: >>>>> >>>>> > https://reviews.apache.org/r/20099/#review46557 >>>>> > ----------------------------------------------------------- >>>>> >>>>> > >>>>> > >>>>> > Alex, one small thing. >>>>> > >>>>> > Just noticed that in the API commands you pass regionUUID as a >>>>> string. You should pass it as a type of UUID and specify the entityType >>>>> parameter in @Parameter so the entity validation is done correctly. >>>>> Example: >>>>> > >>>>> > @Parameter(name=ApiConstants.ZONE_ID, type=CommandType.UUID, >>>>> entityType = ZoneResponse.class, >>>>> > required=true, description="the Zone ID for the network") >>>>> > private Long zoneId; >>>>> > >>>>> > That is the rule when passing id/uuid of the first class CS object >>>>> to the API call >>>>> > >>>>> > Then be aware of the fact that the APIDispatcher will transform UUID >>>>> to the actual DB id, and that would be the Id that you pass to the >>>>> services >>>>> call. If what you need is UUID, not the actual id, to be saved in the >>>>> callContext, you have to transform it explicitly. >>>>> > >>>>> > - Alena Prokharchyk >>>>> > >>>>> > >>>>> >>>>> >> On June 24, 2014, 3:54 p.m., Alex Ough wrote: >>>>> >> >>>>> >> ----------------------------------------------------------- >>>>> >>>>> >> This is an automatically generated e-mail. To reply, visit: >>>>> >> https://reviews.apache.org/r/20099/ >>>>> >>>>> >> ----------------------------------------------------------- >>>>> >> >>>>> >> (Updated June 24, 2014, 3:54 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 0fa3cd5 >>>>> >>>>> >> api/src/com/cloud/user/AccountService.java eac8a76 >>>>> >> api/src/com/cloud/user/DomainService.java 4c1f93d >>>>> >> api/src/org/apache/cloudstack/api/ApiConstants.java adda5f4 >>>>> >> api/src/org/apache/cloudstack/api/BaseCmd.java ac9a208 >>>>> >> >>>>> >>>>> api/src/org/apache/cloudstack/api/command/admin/account/CreateAccountCmd.java >>>>> 50d67d9 >>>>> >> >>>>> >>>>> api/src/org/apache/cloudstack/api/command/admin/account/DeleteAccountCmd.java >>>>> 5754ec5 >>>>> >> >>>>> >>>>> api/src/org/apache/cloudstack/api/command/admin/account/DisableAccountCmd.java >>>>> 3e5e1d3 >>>>> >> >>>>> >>>>> api/src/org/apache/cloudstack/api/command/admin/account/EnableAccountCmd.java >>>>> f30c985 >>>>> >> >>>>> >>>>> api/src/org/apache/cloudstack/api/command/admin/account/LockAccountCmd.java >>>>> 3c185e4 >>>>> >> >>>>> >>>>> api/src/org/apache/cloudstack/api/command/admin/account/UpdateAccountCmd.java >>>>> a7ce74a >>>>> >> >>>>> >>>>> api/src/org/apache/cloudstack/api/command/admin/domain/CreateDomainCmd.java >>>>> 312c9ee >>>>> >> >>>>> >>>>> api/src/org/apache/cloudstack/api/command/admin/domain/DeleteDomainCmd.java >>>>> a6d2b0b >>>>> >> >>>>> >>>>> api/src/org/apache/cloudstack/api/command/admin/domain/UpdateDomainCmd.java >>>>> 409a84d >>>>> >> >>>>> 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/command/admin/user/CreateUserCmd.java >>>>> 8f223ac >>>>> >> >>>>> api/src/org/apache/cloudstack/api/command/admin/user/DeleteUserCmd.java >>>>> 08ba521 >>>>> >> >>>>> api/src/org/apache/cloudstack/api/command/admin/user/DisableUserCmd.java >>>>> c6e09ef >>>>> >> >>>>> api/src/org/apache/cloudstack/api/command/admin/user/EnableUserCmd.java >>>>> d69eccf >>>>> >> >>>>> api/src/org/apache/cloudstack/api/command/admin/user/LockUserCmd.java >>>>> 69623d0 >>>>> >> >>>>> api/src/org/apache/cloudstack/api/command/admin/user/RegisterCmd.java >>>>> 2090d21 >>>>> >> >>>>> api/src/org/apache/cloudstack/api/command/admin/user/UpdateUserCmd.java >>>>> f21e264 >>>>> >> api/src/org/apache/cloudstack/api/response/RegionResponse.java >>>>> 6c74fa6 >>>>> >> 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 29fef4f >>>>> >> >>>>> >>>>> engine/schema/resources/META-INF/cloudstack/core/spring-engine-schema-core-daos-context.xml >>>>> 2ef0d20 >>>>> >> engine/schema/src/com/cloud/user/AccountVO.java 0f5a044 >>>>> >> engine/schema/src/org/apache/cloudstack/region/RegionVO.java >>>>> 608bd2b >>>>> >> >>>>> >>>>> plugins/network-elements/juniper-contrail/test/org/apache/cloudstack/network/contrail/management/MockAccountManager.java >>>>> 4136b5c >>>>> >> plugins/pom.xml b5e6a61 >>>>> >> >>>>> >>>>> plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java >>>>> b753952 >>>>> >> >>>>> >>>>> plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java >>>>> 6f7be90 >>>>> >>>>> >> server/src/com/cloud/api/ApiResponseHelper.java f1f0d2c >>>>> >> server/src/com/cloud/api/dispatch/ParamProcessWorker.java 1592b93 >>>>> >> server/src/com/cloud/event/ActionEventUtils.java 2b3cfea >>>>> >> server/src/com/cloud/projects/ProjectManagerImpl.java d10c059 >>>>> >> server/src/com/cloud/user/AccountManager.java 194c5d2 >>>>> >> server/src/com/cloud/user/AccountManagerImpl.java 7a889f1 >>>>> >> 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/user/AccountManagerImplTest.java 176cf1d >>>>> >> server/test/com/cloud/user/MockAccountManagerImpl.java 746fa1b >>>>> >> server/test/com/cloud/user/MockDomainManagerImpl.java 7dddefb >>>>> >> server/test/org/apache/cloudstack/region/RegionManagerTest.java >>>>> d7bc537 >>>>> >> setup/db/db/schema-440to450.sql ee419a2 >>>>> >> ui/scripts/regions.js 368c1bf >>>>> >> >>>>> >> 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 >>>>> > >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>> >>>> >>> >> >
