They are not called outside and only called from 'subscriber' classes and
FullScanner class.
Do you think these name changes are ok?
- BaseInterface - UserInterface, AccountInterface, DomainInterface
=> APICaller - APIUserCaller, APIAccountCaller, APIDomainCaller
- BaseService - UserService, AccountService, DomainService
=> RemoteResourceProcessor - RemoteUserProcessor,
RemoteAccountProcessor, RemoteDomainProcessor
- FullSyncProcessor - UserFullSyncProcessor, AccountFullSyncProcessor,
DomainFullSyncProcessor
=> no change
- RemoteEventProcessor - RemoteUserEventProcessor,
RemoteAccountEventProcessor, RemoteDomainEventProcessor
=> no change
Thanks
Alex Ough
On Thu, Mar 13, 2014 at 3:58 PM, Alena Prokharchyk <
alena.prokharc...@citrix.com> wrote:
> You extract stuff into interfaces when the methods are meant to be called
> from different classes/Managers. Do you implement to add APIs for your
> plugins? Can your plugin be used by any other CS manager - RegionManager
> for example? If the answer is yes, then you would need an interface. If
> not, abstract class is fine, just remove Interface/Service from the name.
> Also rename your classes to reflect the purpose they are developed for;
> account/user/domain is way too generic and already used in other CS
> packages.
>
>
>
>
>
> On 3/13/14, 1:50 PM, "Alex Ough" <alex.o...@sungard.com> wrote:
>
> >Patch B,
> >
> >1. The reason why I use abstract classes instead of interfaces is because
> >there are some basic methods that are used among the inherited classes, so
> >I'm not sure why it has to be an interface.
> >
> >2. These are the abstract base classes along with their inherited classes
> >and they are grouped by their behavior.
> > - BaseInterface - UserInterface, AccountInterface, DomainInterface
> > - BaseService - UserService, AccountService, DomainService
> > - FullSyncProcessor - UserFullSyncProcessor, AccountFullSyncProcessor,
> >DomainFullSyncProcessor
> > - RemoteEventProcessor - RemoteUserEventProcessor,
> >RemoteAccountEventProcessor, RemoteDomainEventProcessor
> >
> > => Do you recommend to refactor them into defining interfaces and
> >creating one class implementing all methods related with each user,
> >account
> >and domain?
> >
> >3. Let me work on changing names.
> >
> >Thanks
> >Alex Ough
> >
> >
> >
> >
> >On Thu, Mar 13, 2014 at 3:14 PM, Alena Prokharchyk <
> >alena.prokharc...@citrix.com> wrote:
> >
> >> Alex, see inline.
> >>
> >> -Alena.
> >>
> >> From: Alex Ough <alex.o...@sungard.com>
> >> Date: Thursday, March 13, 2014 at 1:00 PM
> >> To: Alena Prokharchyk <alena.prokharc...@citrix.com>
> >> Cc: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Chiradeep
> >> Vittal <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> 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
> >>> >>>>>
> >>> >>>>>
> >>> >>>>
> >>> >>>
> >>> >>
> >>>
> >>>
> >>
>
>
