Hi Alena,
It was a nice to actually meet you during the conference and I updated the
review requests, so please review them and let me know if there is anything
not expected.
Thanks
Alex Ough
On Thu, Apr 3, 2014 at 1:01 PM, Alex Ough <alex.o...@sungardas.com> wrote:
> 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
>>>>>>> >> >>> >>>>>
>>>>>>> >> >>> >>>>>
>>>>>>> >> >>> >>>>
>>>>>>> >> >>> >>>
>>>>>>> >> >>> >>
>>>>>>> >> >>>
>>>>>>> >> >>>
>>>>>>> >> >>
>>>>>>> >>
>>>>>>> >>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
