cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Ough <alex.o...@sungardas.com>
Subject Re: Review Request 20099: Domain-Account-User Sync Up Among Multiple Regions (Core Changes)
Date Fri, 27 Jun 2014 11:41:31 GMT
Kishan,

1. Why Long instead of Integer : You replied that it should be Integer
2. @Encrypt : Does it both encrypt & decrypt? Is there anything necessary
to make it work because it doesn't seem to work when I trace the persist.

Thanks
Alex Ough


On Fri, Jun 27, 2014 at 7:39 AM, Kishan Kavala <Kishan.Kavala@citrix.com>
wrote:

>  Alex,
>
> Are the questions on review board?
>
>
>
> *From:* Alex Ough [mailto:alex.ough@sungardas.com]
> *Sent:* Friday, 27 June 2014 12:03 AM
> *To:* Alena Prokharchyk
> *Cc:* Kishan Kavala; 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)
>
>
>
> 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.Prokharchyk@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.ough@sungardas.com>
> *Date: *Wednesday, June 25, 2014 at 9:03 PM
>
>
> *To: *Alena Prokharchyk <alena.prokharchyk@citrix.com>
> *Cc: *Kishan Kavala <Kishan.Kavala@citrix.com>, "dev@cloudstack.apache.org"
> <dev@cloudstack.apache.org>, Murali Reddy <Murali.Reddy@citrix.com>, Ram
> Ganesh <Ram.Ganesh@citrix.com>, Animesh Chaturvedi <
> animesh.chaturvedi@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.Prokharchyk@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.ough@sungardas.com>
> *Date: *Wednesday, June 25, 2014 at 7:12 PM
>
>
> *To: *Alena Prokharchyk <alena.prokharchyk@citrix.com>
> *Cc: *Kishan Kavala <Kishan.Kavala@citrix.com>, "dev@cloudstack.apache.org"
> <dev@cloudstack.apache.org>, Murali Reddy <Murali.Reddy@citrix.com>, Ram
> Ganesh <Ram.Ganesh@citrix.com>, Animesh Chaturvedi <
> animesh.chaturvedi@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.Prokharchyk@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.ough@sungardas.com>
> *Date: *Wednesday, June 25, 2014 at 6:33 PM
> *To: *Alena Prokharchyk <alena.prokharchyk@citrix.com>
> *Cc: *Kishan Kavala <Kishan.Kavala@citrix.com>, "dev@cloudstack.apache.org"
> <dev@cloudstack.apache.org>, Murali Reddy <Murali.Reddy@citrix.com>, Ram
> Ganesh <Ram.Ganesh@citrix.com>, Animesh Chaturvedi <
> animesh.chaturvedi@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.Prokharchyk@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.ough@sungardas.com>
> *Date: *Wednesday, June 25, 2014 at 4:02 PM
> *To: *Kishan Kavala <Kishan.Kavala@citrix.com>
> *Cc: *Alena Prokharchyk <alena.prokharchyk@citrix.com>, "
> dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Murali Reddy <
> Murali.Reddy@citrix.com>, Ram Ganesh <Ram.Ganesh@citrix.com>, Animesh
> Chaturvedi <animesh.chaturvedi@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.Kavala@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.ough@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.Kavala@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.ough@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.ough@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.Prokharchyk@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.ough@sungardas.com>
> *Date: *Tuesday, June 24, 2014 at 5:41 PM
>
>
> *To: *Alena Prokharchyk <alena.prokharchyk@citrix.com>
> *Cc: *"dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Kishan
> Kavala <Kishan.Kavala@citrix.com>, Murali Reddy <Murali.Reddy@citrix.com>,
> Ram Ganesh <Ram.Ganesh@citrix.com>, Animesh Chaturvedi <
> animesh.chaturvedi@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.Prokharchyk@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.ough@sungardas.com>
> *Date: *Tuesday, June 24, 2014 at 5:17 PM
> *To: *Alena Prokharchyk <alena.prokharchyk@citrix.com>
> *Cc: *"dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Kishan
> Kavala <Kishan.Kavala@citrix.com>, Murali Reddy <Murali.Reddy@citrix.com>,
> Ram Ganesh <Ram.Ganesh@citrix.com>, Animesh Chaturvedi <
> animesh.chaturvedi@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.Prokharchyk@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.ough@sungardas.com>
> *Date: *Tuesday, June 24, 2014 at 4:22 PM
> *To: *"dev@cloudstack.apache.org" <dev@cloudstack.apache.org>
> *Cc: *Alena Prokharchyk <alena.prokharchyk@citrix.com>, Kishan Kavala <
> Kishan.Kavala@citrix.com>, Murali Reddy <Murali.Reddy@citrix.com>, Ram
> Ganesh <Ram.Ganesh@citrix.com>, Animesh Chaturvedi <
> animesh.chaturvedi@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.chaturvedi@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.prokharchyk@citrix.com>
> *Date: *Tuesday, June 24, 2014 at 2:54 PM
> *To: *Alex Ough <alex.ough@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.ough@sungardas.com>
> *Date: *Tuesday, June 24, 2014 at 2:33 PM
> *To: *Alena Prokharchyk <alena.prokharchyk@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.Prokharchyk@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.ough@sungardas.com>
> *Date: *Tuesday, June 24, 2014 at 1:57 PM
>
>
> *To: *Alena Prokharchyk <alena.prokharchyk@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.Prokharchyk@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.ough@sungardas.com>
> *Date: *Tuesday, June 24, 2014 at 12:47 PM
>
>
> *To: *Alena Prokharchyk <alena.prokharchyk@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.ough@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.Prokharchyk@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.prokharchyk@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
> >
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message