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
>>> >>>>>
>>> >>>>>
>>> >>>>
>>> >>>
>>> >>
>>>
>>>
>>
