Well, I'm not sure about that because the help is about how to use @Inject
in the Spring framework.
On Thu, Apr 3, 2014 at 12:49 PM, Alena Prokharchyk <
alena.prokharc...@citrix.com> wrote:
> Alex, please feel free to update Wiki docs related to the questions you
> got Darren's help from. I think this info would be useful for all CS
> committers.
>
> Thank you,
> Alena.
>
> From: Alex Ough <alex.o...@sungardas.com>
> Date: Thursday, April 3, 2014 at 9:22 AM
> To: Chiradeep Vittal <chiradeep.vit...@citrix.com>, Alena Prokharchyk <
> alena.prokharc...@citrix.com>, Darren Shepherd <darren.sheph...@citrix.com
> >
> Cc: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>
>
> Subject: Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up
> Among Multiple Regions
>
> Forgot to mention this, but it was a great help from Darren.
> Thanks again, Darren!
> Alex Ough
>
>
> On Thu, Apr 3, 2014 at 11:56 AM, Alex Ough <alex.o...@sungardas.com>wrote:
>
>> All,
>>
>> I updated the patches as per Alena's request.
>>
>> Let me know if there is anything missing/incorrect.
>> Thanks
>> Alex Ough
>>
>>
>> On Wed, Mar 26, 2014 at 1:40 PM, Alex Ough <alex.o...@sungard.com> wrote:
>>
>>> Sorry, my bad. I mis-read your comment.
>>>
>>> Thanks for pointing it out.
>>> Alex Ough
>>>
>>>
>>> On Wed, Mar 26, 2014 at 12:25 PM, Chiradeep Vittal <
>>> chiradeep.vit...@citrix.com> wrote:
>>>
>>>> I didn't say "do not use auto wiring". I said the convention is to
>>>> use @Inject which you didn't have.
>>>>
>>>> From: Alena Prokharchyk <alena.prokharc...@citrix.com>
>>>> Date: Wednesday, March 26, 2014 at 7:39 AM
>>>> To: Alex Ough <alex.o...@sungard.com>, "dev@cloudstack.apache.org" <
>>>> dev@cloudstack.apache.org>
>>>> Cc: Chiradeep Vittal <chiradeep.vit...@citrix.com>
>>>>
>>>> Subject: Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up
>>>> Among Multiple Regions
>>>>
>>>> Alex, I'm attending a conference today, will review the patch
>>>> tomorrow.
>>>>
>>>> -Alena
>>>>
>>>> From: Alex Ough <alex.o...@sungard.com>
>>>> Date: Wednesday, March 26, 2014 at 6:35 AM
>>>> To: Alena Prokharchyk <alena.prokharc...@citrix.com>
>>>> Cc: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Chiradeep
>>>> Vittal <chiradeep.vit...@citrix.com>
>>>> Subject: Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up
>>>> Among Multiple Regions
>>>>
>>>> Alena,
>>>>
>>>> It took a little time to update the patch because I had a vacation
>>>> last week.
>>>> Now I uploaded the updated patch for review with below status, so
>>>> please check them and let me know what you think.
>>>> I hope it to be merged soon to wrap this up asap.
>>>>
>>>> 1. no change with waiting for comments on my recommendation.
>>>>
>>>> 2. The two things to turn on the sync-up among multiple regions is to
>>>> change the class name of "eventNotificationBus" into "MultiRegionEventBus"
>>>> from "RabbitMQEventBus" as below and change the value of
>>>> 'region.full.scan.interval' in Global Settings. And the new classes are
>>>> never used unless the feature is turned on, so I'm not sure if auto wiring
>>>> is necessary here and Chiradeep asked not to use @inject in his initial
>>>> review, so I removed them.
>>>> <bean id="eventNotificationBus"
>>>> class="org.apache.cloudstack.mom.rabbitmq.MultiRegionEventBus">
>>>>
>>>> 3. They are not adaptors, but inherited classes to process
>>>> domain/account/user objects separately.
>>>>
>>>> 4. Done
>>>>
>>>> 5. Same
>>>>
>>>> 6. I removed 'domain path' from AccountResponse & UserResponse.
>>>>
>>>> Thanks
>>>> Alex Ough
>>>>
>>>>
>>>> On Thu, Mar 13, 2014 at 9:48 PM, Alex Ough <alex.o...@sungard.com>wrote:
>>>>
>>>>> What I think based on your comments are
>>>>>
>>>>> 1. Well, I have a different thought. I'd rather have separated
>>>>> classes instead of having a class with lots of methods, which makes the
>>>>> maintenance easier. And as you show as an example, it is possible to
>>>>> create
>>>>> a method and merge them, but I think it is better to provide separate
>>>>> methods that are exposed outside so that the callers can know what to call
>>>>> with ease.
>>>>>
>>>>> 2. Let me look at that.
>>>>>
>>>>> 3. I'm not sure why you think they are adapters, but let me look at
>>>>> that class.
>>>>>
>>>>> 4. OK, let me work on this.
>>>>>
>>>>> 5. The urls are stored in the region table along with username and
>>>>> password. Please see 'RegionVO' under
>>>>> 'engine/schema/src/org/apache/cloudstack/region'.
>>>>>
>>>>> Thanks
>>>>> Alex Ough
>>>>>
>>>>>
>>>>> On Thu, Mar 13, 2014 at 5:49 PM, Alena Prokharchyk <
>>>>> alena.prokharc...@citrix.com> wrote:
>>>>>
>>>>>> Alex,
>>>>>>
>>>>>> There are so many classes, and it makes it hard to see/review the
>>>>>> feature.
>>>>>> Can you come up with some sort of visual diagram, so its easier to see
>>>>>> which component is responsible for what task, and how they interact
>>>>>> with
>>>>>> each other.
>>>>>>
>>>>>> My suggestions:
>>>>>>
>>>>>> 1) I think it would make sense to merge all the classes doing utils
>>>>>> tasks
>>>>>> (forming and sending Apis to CS) - UserInterface, AccountInterface,
>>>>>> DomainInterface - to a single util class. They do return generic types
>>>>>> anyway - JsonArray/JsonObject, and they do perform a generic util
>>>>>> task -
>>>>>> forming and sending the request to the CS. I would merge them all
>>>>>> with the
>>>>>> BaseInterface and name it with the name indicating that this class is
>>>>>> responsible for sending API calls against CS. And this would be your
>>>>>> util
>>>>>> class.
>>>>>>
>>>>>>
>>>>>> You should also come up with some generic method that forms the
>>>>>> requests
>>>>>> to CS APIs to make the code cleaner.
>>>>>>
>>>>>> For example, look at these 2:
>>>>>>
>>>>>>
>>>>>> public JSONObject lockUser(String userId) throws Exception {
>>>>>> String paramStr = "command=lockUser&id=" + userId +
>>>>>> "&response=json&sessionkey=" + URLEncoder.encode(getSessionKey(),
>>>>>> "UTF-8");
>>>>>> return sendApacheGet(paramStr);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> public JSONObject disableUser(String userId) throws Exception {
>>>>>>
>>>>>> String paramStr = "command=disableUser&id=" + userId +
>>>>>> "&response=json&sessionkey=" + URLEncoder.encode(getSessionKey(),
>>>>>> "UTF-8");
>>>>>> return sendApacheGet(paramStr);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> You repeat appending json and session key in all of them. Why not
>>>>>> create
>>>>>> some generic method where you pass a) commandName 2) map of
>>>>>> parameters,
>>>>>> and make that method return JsonObject/JsonArray?
>>>>>>
>>>>>>
>>>>>> 2) I would suggest you utilize Spring framework in your code and auto
>>>>>> wire
>>>>>> all the dependencies by using @Inject rather than locating them with
>>>>>> the
>>>>>> components lifecycle. Please refer to Apache Wiki:
>>>>>>
>>>>>>
>>>>>> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Using+Spring+in+Clou
>>>>>> dStack
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> 3) AccountFullSyncProcessor/DomainFullSyncProcessor/<other
>>>>>> processors>.
>>>>>> These looks like adaptors to me. Have you looked at how AdapterBase is
>>>>>> utilized in CS? You should take a look at it.
>>>>>>
>>>>>>
>>>>>> 4) I see that you have a folder called "simulator". Does this folder
>>>>>> contain Test classes used by some kind of simulator? Or would they be
>>>>>> used
>>>>>> in production? If its just for testing, please rename them by putting
>>>>>> the
>>>>>> word "Simulator" in the name. Look at how other simulator classes are
>>>>>> named in CS (SimulatorImageStoreLifeCycleImpl.java for example)
>>>>>>
>>>>>> 5) In the code, I haven't noticed where you store/read the end point
>>>>>> URL
>>>>>> to the CS Management server - the server address you are gonna send
>>>>>> your
>>>>>> requests to. If for example, you have MS1 and MS2, will your plugin
>>>>>> from
>>>>>> MS1 ever sends a request to MS2? And if it sends the request only to
>>>>>> MS1,
>>>>>> what ip address it uses?
>>>>>>
>>>>>>
>>>>>>
>>>>>> -Alena.
>>>>>>
>>>>>> On 3/13/14, 2:58 PM, "Alex Ough" <alex.o...@sungard.com> wrote:
>>>>>>
>>>>>> >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
>>>>>> >> >>> >>>>>
>>>>>> >> >>> >>>>>
>>>>>> >> >>> >>>>
>>>>>> >> >>> >>>
>>>>>> >> >>> >>
>>>>>> >> >>>
>>>>>> >> >>>
>>>>>> >> >>
>>>>>> >>
>>>>>> >>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
