Sounds like it goes back to what I said.... I wish they have been involved more actively from the start.
Thanks but really making me tired. Alex Ough On Thu, Jun 26, 2014 at 5:17 PM, Alena Prokharchyk < alena.prokharc...@citrix.com> wrote: > I did logic review according to the FS assuming that the FS (and the > design described there) was approved on the [PROPOSAL] stage, BEFORE the > code was put it to the review board. Was it approved at that stage? > > Alex, the feature is not small, and considering that it raised so many > questions and arguing, I would really like to get a final design/logic > review + “ship it” from people having expertise on the topic, and/or who > originally participated in review/discussion: Chiradeep, Kishan, Murail. > > Thank you, > Alena. > > From: Alex Ough <alex.o...@sungardas.com> > Date: Thursday, June 26, 2014 at 1:53 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, > Didn't you say that you guys already "did logic review" in the previous > email? > > Thanks > Alex Ough > > > On Thu, Jun 26, 2014 at 2:59 PM, Alena Prokharchyk < > alena.prokharc...@citrix.com> wrote: > >> Alex, sorry to hear that it took so long to get on the review process. >> The question still remains – before you started working on implementation, >> and posted your plugin’s code, was the FS approved/reviewed as a part of >> [PROPOSAL] discussion? We should never start the development until you get >> the input from the community on the FS and confirm that the design is valid >> and the feature can contribute to CS. Only after the proposal is accepted, >> you can request the Reviewboard ticket review. So I did assume that the >> [PROPOSAL] phase was finished, and the FS was validated as a part of it, >> when I was asked by Daan to review the Reviewboard ticket. >> >> I’ve also looked at the history. I can see that Chiradeep contributed >> to the design/plugin logic discussion as well as pointed to the changes >> that need to be done to the code structure. I helped to review the second. >> >> Lets wait for the update from Kishan. Kishan, in addition to answering >> Alex’s questions, please go over the plugin design once again. >> >> Thank you, >> Alena. >> >> From: Alex Ough <alex.o...@sungardas.com> >> Date: Thursday, June 26, 2014 at 11:32 AM >> >> 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, >> >> 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 >>>>>>> > >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> >
