Alex, see inline. -Alena.
From: Alex Ough <alex.o...@sungard.com<mailto:alex.o...@sungard.com>> Date: Thursday, March 13, 2014 at 1:00 PM To: Alena Prokharchyk <alena.prokharc...@citrix.com<mailto:alena.prokharc...@citrix.com>> Cc: "dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>" <dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>>, Chiradeep Vittal <chirade...@gmail.com<mailto:chirade...@gmail.com>> Subject: Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among Multiple Regions Hi Alena, Patch B, I'm not quite familiar with java, so I have a little difficulty in following your recommendation. Can you send me an example using 'BaseInterface' and/or 'AccountInterface'? * What is called an interface in java: http://docs.oracle.com/javase/tutorial/java/concepts/interface.html. Its a place where all your methods are defined w/o actual implementation. * Look at any of cloudStack Managers implementation. Take for example: 1. AcccountManagerImpl.java – class where all the methods are implemented. Part of the server package 2. AccountManagerImpl implements 2 interfaces – AccountManager and AccountService. If you want any of your methods to be used by plugins/services, define them in Service interface. If all of them are meant to be used just inside your plugin/or cloudstack core/server – define them in Manager interface. 3. I would suggest you rename your classes/interfaces by adding your feature specific keyword to the name. CloudStack already has AccountService interface. And BaseInterface name is way too generic. Plus you shouldn’t really put an “Interface” to the name. It will be very helpful and appreciated. Patch A, To reduce the number of requests to the remote regions because the syncing is always using the api requests a lot to get information of domains/accounts/users from remote regions. * you can’t ,modify cloudStack core/server code only to fix problem that is specific to your plugin/service. The solution for your case will be – create in memory data structure where you keep account/domain information. Account->domain relationship don’t change along account lifecycle, as well as the domain path doesn’t change for the domain once its created. And get the domain path from there. And let me change the concatenation into using StringBuilder. Thanks a lot for your reply. Alex Ough On Thu, Mar 13, 2014 at 2:41 PM, Alena Prokharchyk <alena.prokharc...@citrix.com<mailto:alena.prokharc...@citrix.com>> wrote: Alex, I have some comments. Patch B. * You shouldn’t make your service a part of cloud-mom-rabbitmq plugin. Your subscribers/implementation are specific to your feature, and you need to introduce a special plugin just for your service. * AccountInterface and BaseInterface are still regular classes. You should split them into Service interface /ManagerImpl or Manager interface /ManagerImpl as Chiradeep suggested. * Once you extract services interfaces, make sure you don’t use VO objects in methods signatures. * You should really get a use of @Manager interface and @Inject annotations for autowiring your managers instead of setting them up using ComponentContext.getComponent() in each of your manager classes. Patch A. * AccountResponse. Why did you add domain path to the account response? You can always retrieve it by a) get domain info from account response b) execute listDomains&domainId to get the domain path. Try not to overload the response with attributes that don’t belong to response’s first class object. Generic comments. * I can see that you do a lot of string concatenation this way: paramStr += "&email=" + email + "&firstname=" + firstName + "&lastname=" + lastName + "&accounttype=" + accountType; I would suggest to use StringBuilder instead. On 3/13/14, 9:33 AM, "Alex Ough" <alex.o...@sungard.com<mailto:alex.o...@sungard.com>> wrote: >Chiradeep, > >Any comments on them? > >Thanks >Alex Ough > > >On Wed, Mar 12, 2014 at 10:58 AM, Alex Ough ><alex.o...@sungard.com<mailto:alex.o...@sungard.com>> wrote: > >> And I also uploaded the patch B that includes new implementation to >> support multi regions. >> >> Thanks >> Alex Ough >> >> >> On Wed, Mar 12, 2014 at 10:17 AM, Alex Ough >> <alex.o...@sungard.com<mailto:alex.o...@sungard.com>> >>wrote: >> >>> I uploaded the patch A that includes only core changes, so please >>>review >>> it and let me know if you have any comments. >>> >>> Thanks >>> Alex Ough >>> >>> >>> On Wed, Mar 12, 2014 at 8:24 AM, Alex Ough >>> <alex.o...@sungard.com<mailto:alex.o...@sungard.com>> >>>wrote: >>> >>>> Hi Chiradeep, >>>> >>>> Can you give me the example of the Singleton service class you >>>>mentioned? >>>> I'm not sure if you are asking the name changes or else because those >>>> classes are abstract classes and do not need to be singleton class. >>>> >>>> And let me try the refactoring and ask confirmations to you, so I >>>>hope I >>>> can get the reply soon. >>>> >>>> Thanks >>>> Alex Ough >>>> >>>> >>>> On Wed, Mar 12, 2014 at 4:13 AM, Daan Hoogland >>>><daan.hoogl...@gmail.com<mailto:daan.hoogl...@gmail.com>>wrote: >>>> >>>>> H Alex, I considder Chiradeeps comments quite valid and serious >>>>>enough >>>>> to anticipate not making friday 14:00 CET. That would mean no merge >>>>> before 4.4. Can you live with that? >>>>> >>>>> On Wed, Mar 12, 2014 at 6:40 AM, Chiradeep Vittal >>>>> <chiradeep.vit...@citrix.com<mailto:chiradeep.vit...@citrix.com>> wrote: >>>>> > Hi Alex, >>>>> > >>>>> > If you look at the general design of CloudStack, there are >>>>>Singleton >>>>> > service interfaces which are then implemented with real classes. >>>>>This >>>>> > facilitates easy testing by mocking the interface. In your new >>>>>files >>>>> > (BaseInterface, which by the way is NOT an interface, >>>>>AccountService, >>>>> > which is NOT a service like other CloudStack services, and so on), >>>>> this >>>>> > design paradigm is not followed. Therefore it is incongruous to the >>>>> rest >>>>> > of the code base. >>>>> > >>>>> > Furthermore this has been plopped right in the middle of other core >>>>> > CloudStack code (server/src/com/cloud/region/). If you look at the >>>>> > EventBus logic, it has been written as a plugin, thereby enabling >>>>> users >>>>> > who do not need it to disable it. This level of configuration is >>>>> needed >>>>> > and I can't find it. >>>>> > >>>>> > So, to summarize: >>>>> > 1. Split it into patches. Patch A is the change to the core DAOs, >>>>>db >>>>> > logic, schema upgrade code, etc. >>>>> > 2. Patch B is the actual sync service written as an optional plugin >>>>> with >>>>> > the package name space of org.apache. >>>>> > >>>>> > On 3/11/14, 3:09 PM, "Alex Ough" >>>>> > <alex.o...@sungard.com<mailto:alex.o...@sungard.com>> wrote: >>>>> > >>>>> >>Hi Daan, >>>>> >> >>>>> >>I didn't update the patch after the couple of works because I >>>>>wanted >>>>> to do >>>>> >>it with others Chiradeep asked if any. >>>>> >>Let me know when you want me to. >>>>> >> >>>>> >>Thanks >>>>> >>Alex Ough >>>>> >> >>>>> >> >>>>> >>On Tue, Mar 11, 2014 at 4:37 PM, Daan Hoogland >>>>> >><daan.hoogl...@gmail.com<mailto:daan.hoogl...@gmail.com>>wrote: >>>>> >> >>>>> >>> I will call the merge in a moment, but won't do it immediately. >>>>> >>> >>>>> >>> Be there for me when any issues come up. If not I will revert it. >>>>> >>> >>>>> >>> @Chiradeep: did Alex answer your concerns adequately? >>>>> >>> >>>>> >>> >>>>> >>> kind regards, >>>>> >>> >>>>> >>> On Tue, Mar 11, 2014 at 6:43 PM, Alex Ough >>>>><alex.o...@sungard.com<mailto:alex.o...@sungard.com>> >>>>> >>>wrote: >>>>> >>> > I worked on a couple of items, so can you give me the >>>>> confirmation for >>>>> >>> the >>>>> >>> > rest items so that I can wrap this up? >>>>> >>> > I really want to include this into 4.4. >>>>> >>> > >>>>> >>> > Thanks >>>>> >>> > Alex Ough >>>>> >>> > >>>>> >>> > >>>>> >>> > On Mon, Mar 10, 2014 at 3:41 PM, Alex Ough >>>>><alex.o...@sungard.com<mailto:alex.o...@sungard.com> >>>>> > >>>>> >>> wrote: >>>>> >>> > >>>>> >>> >> Please see my reply/question in blue. >>>>> >>> >> >>>>> >>> >> Thanks >>>>> >>> >> Alex Ough >>>>> >>> >> >>>>> >>> >> >>>>> >>> >> On Mon, Mar 10, 2014 at 1:25 PM, Chiradeep Vittal < >>>>> >>> >> chiradeep.vit...@citrix.com<mailto:chiradeep.vit...@citrix.com>> >>>>> >>> >> wrote: >>>>> >>> >> >>>>> >>> >>> I haven¹t looked at it because it is huge. But preliminary >>>>>scan: >>>>> >>> >>> >>>>> >>> >>> - there are unit tests missing for changes to core CS code >>>>> >>> >>> >>>>> >>> >> Unit tests for only new classes were added because I >>>>> >>>couldn't >>>>> >>> >> find unit tests of the ones I modified >>>>> >>> >> >>>>> >>> >> >>>>> >>> >>> - uses com.amazonaws.util.json (why?) >>>>> >>> >>> >>>>> >>> >> They are used to handle the json objects. Let me know >>>>> if you >>>>> >>> >> want me to replace it with another. >>>>> >>> >> >>>>> >>> >> - code format does not follow coding convention ( placement of >>>>> {}, >>>>> >>>camel >>>>> >>> >>> case api_interface ) >>>>> >>> >>> >>>>> >>> >> Done >>>>> >>> >> >>>>> >>> >> >>>>> >>> >>> - package namespace is com.cloud instead of org.apache for >>>>>new >>>>> files >>>>> >>> >>> >>>>> >>> >> I didn't know that. So do I need to use 'org.apache' >>>>> package >>>>> >>> for >>>>> >>> >> all new classes? >>>>> >>> >> >>>>> >>> >> - no file-level comments. What does LocalAccountManager do? >>>>>Why >>>>> does >>>>> >>>it >>>>> >>> >>> exist? Etc. >>>>> >>> >>> >>>>> >>> >> Done. >>>>> >>> >> >>>>> >>> >> >>>>> >>> >>> - No interfaces, only abstract classes. No use of @Inject. >>>>>While >>>>> >>>this >>>>> >>> >>> might be OK, it does make it harder to test and does not >>>>>follow >>>>> the >>>>> >>> rest >>>>> >>> >>> of ACS conventions. >>>>> >>> >>> >>>>> >>> >> I don't think there is any interface, but let me know >>>>>if >>>>> you >>>>> >>> find >>>>> >>> >> any. >>>>> >>> >> Any recommendation to replace @inject? >>>>> >>> >> >>>>> >>> >>> >>>>> >>> >>> I would urge the submitter to break up the submission. >>>>> >>> >>> A) the changes to CS core, with explanations / comments and >>>>> tests >>>>> >>> >>> B) the sync service should be an interface with concrete >>>>> >>> implementations >>>>> >>> >>> in its own package >>>>> >>> >>> C) more tests >>>>> >>> >>> >>>>> >>> >> can you give me a little specific what kind of tests >>>>>you >>>>> need >>>>> >>> >> more? >>>>> >>> >> >>>>> >>> >> >>>>> >>> >> >>>>> >>> >>> On 3/10/14, 3:37 AM, "Daan Hoogland" >>>>><daan.hoogl...@gmail.com<mailto:daan.hoogl...@gmail.com>> >>>>> >>>wrote: >>>>> >>> >>> >>>>> >>> >>> >Hi everyody, >>>>> >>> >>> > >>>>> >>> >>> >The people at sungard have been making this change for 4.4 >>>>>and >>>>> I >>>>> >>>want >>>>> >>> >>> >to merge/apply it this week. It is more then a screenfull >>>>>and >>>>> might >>>>> >>> >>> >cause issue is a setup or two. >>>>> >>> >>> > >>>>> >>> >>> >have a look at https://reviews.apache.org/r/17790/ >>>>> >>> >>> > >>>>> >>> >>> >a ref to ticket and fs page are in the review request. >>>>> >>> >>> > >>>>> >>> >>> >kind regards, >>>>> >>> >>> >-- >>>>> >>> >>> >Daan >>>>> >>> >>> >>>>> >>> >>> >>>>> >>> >>> >>>>> >>> >> >>>>> >>> >>>>> >>> >>>>> >>> >>>>> >>> -- >>>>> >>> Daan >>>>> >>> >>>>> >>> >>>>> > >>>>> >>>>> >>>>> >>>>> -- >>>>> Daan >>>>> >>>>> >>>> >>> >>
