Hi John, Thanks a lot for your detailed feedback, but I strongly suggest to continue to discuss about them after this development is wrapped up because this feature is provided as a plugin and and will not work unless you turn it on in case you don't want to use.
Thanks Alex Ough On Fri, Jun 27, 2014 at 11:49 AM, John Burwell <jburw...@basho.com> wrote: > Alex, > > I appreciate you taking the time to answer my questions since I am late > joining this conversation. I wanted to make sure I understand the > underlying design and its assumptions before commenting in depth. My > detailed followups are in-line below. TL;DR I am concerned that the design > does not properly address the myriad of network partition or conflict > resolution scenarios that will inevitably arise in real world operation. > Furthermore, you did not answer my biggest question — why isn’t something > like Percona master-master replication [1] not sufficient for this > capability? > > I apologize again for missing the original proposal thread as we could > have addressed these issues before code was cut. I would imagine you are > likely frustrated by the length of time it has taken to get this patch into > master. I would like to say you experience is atypical, and my goal is to > help find the best design/solution for multi-region data sync. Finally, > please don’t take my feedback (or anyone else’s) as a lack of appreciate > for your efforts. I come at every review from the perspective that I will > be one of the people responsible for supporting/maintaining it in future > releases. Therefore, I want to ensure that new work does not incur any > technical debt that would hobble the community’s long term development > efforts. > > Thanks, > -John > > On June 27, 2014 at 7:46:21 AM, Alex Ough (alex.o...@sungardas.com) wrote: > > Jonn, > > 1. out of spec. we can add this later if necessary. > 2. out of spec. we can add this later if necessary. > 3. out of spec. we can add this later if necessary. > > I want to understand why these data elements were not included. To my > mind, this feature is functionally incomplete without the inclusion of > project and event data. One of the primary use cases for this capability > will be geographical distribution of applications/system for which sync’ed > project data will be required. Without the synchronization of the event > data, it is impossible to gain a complete operational picture of the > infrastructure being managed. Template data I could accept as being > deferred to a later release though I think this omission will disappoint > many multi-region users. > > Today, we sidestep these issues because we don’t sync anything — each > region runs an independent CloudStack instance owned and operated by the > same organization. However, once we start syncing data between regions, we > need to ensure that the data set is logically complete. If we do not, the > feature will, at best, be cruft the community must maintain and frustrate > users. > > > 4. Whenever there are changes in the records, the time stamps are logged > and the later change wins. > > Timestamps are, perhaps, the most unreliable approach to conflict > resolution. (As an side, Riak defaults to last write wins, and we regret > that decision on a daily basis. So much so that it won’t be the default in > 2.0.) It requires complete time sync across all regions which is > notoriously difficult to achieve. In practice, it typically requires GPS > receivers in each data center. When clocks fall out of sync, data gets > silently corrupted — no errors occur. Therefore, I do not believe we can > accept timestamp based conflict resolution due to high likelihood of data > corruption. > > I highly suggest reading Lamport’s classic Time, Clocks, and the Ordering > of Events in a Distributed System [2] paper for a deeper examination of the > issues with using wall clocks for data synchronization and approaches to > achieving partial event ordering. Thankfully, there are safe approaches to > distributed conflict resolution such as vector clocks[3][4][5], version > vectors[6], and CRDTs [7]. > > > 5. It relies on the order of events, so if the order is reversed with some > reason, the creations will fail, but they will be covered by FullScan. > > First, there is no guaranteed order of message delivery unless the > synchronization mechanism uses a single consumer thread. While this > approach would assure ordered processing, it not would scale sufficiently > (i.e. realtime data sync wouldn’t be on the very long end of eventually > consistent). How does the FullScan operation exchange data between > regions? Also, do you intend for this feature to be a master-master or > master-slave replication model? If it is master-master, a final > reconciliation step will be required by the region initiating the FullScan > operation which a tricky bit to properly implement since changes may occur > in the data between the time the FullScan is initiated and the > reconciliation begins. How does the mechanism handle an interruption of > the FullScan operation (e.g. management server or database crash or network > partition during sync operation)? > > In terms of handling the referential integrity issue, one approach that > could work would be to resubmit the message when a referential integrity > error occurs — assuming that the message for a parent record is either > waiting in the queue or being processed concurrently. Such an approach > must include a retry count and limit to protect against scenarios where the > parent-child relationships can not be resolved and the management server > simply needs to give up. > > 6. It sounds like not related with this project. > > Partition tolerance is absolutely critical to any data synchronization > operation. In CAP terms, you are proposing a available/partition tolerant > mechanism. There will inevitably be network partitions when synchronizing > data across WAN links (as there will be inside a datacenter). How does this > design provide the partition tolerance to ensure correct and complete sync > following periods of network unavailability between regions? I suggest > reading Kyle Kingsbury’s excellent Call Me Maybe [8] series on this > subject. To give away a bit of the ending, RabbitMQ does not provide > proper partition tolerance [9][10]. Without understanding how this > attribute will be fulfilled, the design is, in my view, incomplete. > > > 7. The interval for FullScan processing is configurable in the global > setting, 'region.full.scan.interval'. > > The interval does not address the problem that the system may be be under > high load when the FullScan starts. Without back pressure, this mechanism > could cause an internal denial of service since it may perform full scans > of potentially large tables. Therefore, there should be check in the > FullScan that the system is not too busy to perform the operation. If it > is, then it should skip the FullScan and try again at the next interval. > Also, how does the FullScan operation prevent memory explosion as data > sets grow? > > > > Thanks > Alex Ough > > > [1]: > http://www.percona.com/doc/percona-xtradb-cluster/5.5/features/multimaster-replication.html > [2]: http://web.stanford.edu/class/cs240/readings/lamport.pdf > [3]: http://en.wikipedia.org/wiki/Vector_clock (yes, Wikipedia has a > good, straightforward explanation) > [4]: http://basho.com/why-vector-clocks-are-easy/ > [5]: http://basho.com/why-vector-clocks-are-hard/ > [6]: http://en.wikipedia.org/wiki/Version_vector > [7]: http://pagesperso-systeme.lip6.fr/Marc.Shapiro/papers/RR-6956.pdf > [8]: http://aphyr.com/ > [9]: http://aphyr.com/posts/315-call-me-maybe-rabbitmq > [10]: https://www.rabbitmq.com/partitions.html > > > On Fri, Jun 27, 2014 at 12:02 AM, John Burwell <jburw...@basho.com> wrote: > >> All, >> >> I apologize for joining this conversation late. I understand that this >> patch was submitted back in February. Around this time, my family had a >> significant medical event, and I was disengaged from all work activities — >> missing the original conversation. >> >> Reading through the specification, and briefly reviewing the code, I >> would like to understand the following assumptions/design decisions: >> >> 1. Why aren’t projects being sync’ed? It seems very likely that users >> would want to have projects span data centers for redundancy/DR purposes. >> 2. Why aren’t events being sync’ed? I can imagine a number of >> scenarios where I would want to examine the operation of an logical >> application or system across both regions. Without the sync of event data, >> I would be forced to either perform that interleave visually with two >> browser tabs or dump the data into another datastore to be merged. >> 3. Why isn’t template metadata being sync’ed? When spanning an >> application/system across regions, it would seem to follow that I would >> want to use the same templates. >> 4. How does this design deal with modifications to a record in two or >> more regions during a network partition? >> 5. Given that messages can/will be processed out of order, how is >> referential integrity maintained when a parent and a set of children are >> created (e.g. creation of a new account and a set of users rapidly through >> the API)? >> 6. Is RabbitMQ being relied upon to provide partition tolerance? >> 7. Is there a back pressure mechanism to throttle the full sync >> operation when the database/management server is under heavy load? >> >> Finally, I would like to understand why we are taking on multi-datacenter >> data replication in CloudStack, and not deferring to underlying datastore. >> Speaking as someone whose $dayjob involves delivering such a system (at >> Basho for Riak), it is a very hard thing to get right (there literally >> thousands of corner cases). The design document does not speak to this >> decision, and I would like understand how CloudStack could not leverage >> existing, mature mechanisms at the datastore-level. >> >> I apologize if some of these questions have been answered already. I >> attempt to look back in the archives, but given the span of this >> conversation, it was difficult to piece together retroactively. >> >> Thanks, >> -John >> >> On June 26, 2014 at 5:34:31 PM, Alex Ough (alex.o...@sungardas.com) >> wrote: >> >> 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 >> >>>>>>> > >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>>> >> >>>>>> >> >>>>>> >> >>>>> >> >>>> >> >>> >> >> >> > >> >> > > > >
