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'?
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.
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> 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> wrote:
>
> >Chiradeep,
> >
> >Any comments on them?
> >
> >Thanks
> >Alex Ough
> >
> >
> >On Wed, Mar 12, 2014 at 10:58 AM, Alex Ough <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>
> >>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>
> >>>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>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> 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> 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>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>
> >>>>> >>>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
> >>>>> >
> >>>>> >>> 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> 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>
> >>>>> >>>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
> >>>>>
> >>>>>
> >>>>
> >>>
> >>
>
>
