You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Alex Ough <al...@sungardas.com> on 2014/04/03 17:56:31 UTC
Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among
Multiple Regions
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 <al...@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.Vittal@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 <Al...@citrix.com>
>> Date: Wednesday, March 26, 2014 at 7:39 AM
>> To: Alex Ough <al...@sungard.com>, "dev@cloudstack.apache.org" <
>> dev@cloudstack.apache.org>
>> Cc: Chiradeep Vittal <ch...@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 <al...@sungard.com>
>> Date: Wednesday, March 26, 2014 at 6:35 AM
>> To: Alena Prokharchyk <al...@citrix.com>
>> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>, Chiradeep
>> Vittal <Ch...@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 <al...@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.Prokharchyk@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" <al...@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.Prokharchyk@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" <al...@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.Prokharchyk@citrix.com> wrote:
>>>> >> >
>>>> >> >> Alex, see inline.
>>>> >> >>
>>>> >> >> -Alena.
>>>> >> >>
>>>> >> >> From: Alex Ough <al...@sungard.com>
>>>> >> >> Date: Thursday, March 13, 2014 at 1:00 PM
>>>> >> >> To: Alena Prokharchyk <al...@citrix.com>
>>>> >> >> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>,
>>>> >>Chiradeep
>>>> >> >> Vittal <ch...@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.Prokharchyk@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" <al...@sungard.com> wrote:
>>>> >> >>>
>>>> >> >>> >Chiradeep,
>>>> >> >>> >
>>>> >> >>> >Any comments on them?
>>>> >> >>> >
>>>> >> >>> >Thanks
>>>> >> >>> >Alex Ough
>>>> >> >>> >
>>>> >> >>> >
>>>> >> >>> >On Wed, Mar 12, 2014 at 10:58 AM, Alex Ough <
>>>> alex.ough@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
>>>> >><al...@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
>>>> >><al...@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
>>>> >> >>> >>>><da...@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
>>>> >> >>> >>>>> <Ch...@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" <al...@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
>>>> >> >>> >>>>> >><da...@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
>>>> >> >>> >>>>><al...@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.ough@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.Vittal@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"
>>>> >> >>> >>>>><da...@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
>>>> >> >>> >>>>>
>>>> >> >>> >>>>>
>>>> >> >>> >>>>
>>>> >> >>> >>>
>>>> >> >>> >>
>>>> >> >>>
>>>> >> >>>
>>>> >> >>
>>>> >>
>>>> >>
>>>>
>>>>
>>>
>>
>
Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among
Multiple Regions
Posted by Alena Prokharchyk <Al...@citrix.com>.
Likewise, Alex.
Will review the patch tomorrow.
Thank you!
Alena.
On 4/16/14, 12:11 PM, "Alex Ough" <al...@sungardas.com> wrote:
>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
><al...@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
><Al...@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 <al...@sungardas.com>
>Date: Thursday, April 3, 2014 at 9:22 AM
>To: Chiradeep Vittal <Ch...@citrix.com>, Alena Prokharchyk
><al...@citrix.com>,
> Darren Shepherd <da...@citrix.com>
>Cc: "dev@cloudstack.apache.org" <de...@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
><al...@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
><al...@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
><Ch...@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 <Al...@citrix.com>
>Date: Wednesday, March 26, 2014 at 7:39 AM
>To: Alex Ough <al...@sungard.com>, "dev@cloudstack.apache.org"
><de...@cloudstack.apache.org>
>Cc: Chiradeep Vittal <ch...@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 <al...@sungard.com>
>Date: Wednesday, March 26, 2014 at 6:35 AM
>To: Alena Prokharchyk <al...@citrix.com>
>Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>, Chiradeep
>Vittal <Ch...@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
><al...@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
><Al...@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+Clo
>u
>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" <al...@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.Prokharchyk@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" <al...@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.Prokharchyk@citrix.com> wrote:
>>> >
>>> >> Alex, see inline.
>>> >>
>>> >> -Alena.
>>> >>
>>> >> From: Alex Ough <al...@sungard.com>
>>> >> Date: Thursday, March 13, 2014 at 1:00 PM
>>> >> To: Alena Prokharchyk <al...@citrix.com>
>>> >> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>,
>>>Chiradeep
>>> >> Vittal <ch...@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.Prokharchyk@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" <al...@sungard.com> wrote:
>>> >>>
>>> >>> >Chiradeep,
>>> >>> >
>>> >>> >Any comments on them?
>>> >>> >
>>> >>> >Thanks
>>> >>> >Alex Ough
>>> >>> >
>>> >>> >
>>> >>> >On Wed, Mar 12, 2014 at 10:58 AM, Alex Ough
>>><al...@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
>>><al...@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
>>><al...@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
>>> >>> >>>><da...@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
>>> >>> >>>>> <Ch...@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" <al...@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
>>> >>> >>>>> >><da...@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
>>> >>> >>>>><al...@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.ough@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.Vittal@citrix.com <ma...@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"
>>> >>> >>>>><da...@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/ <https://reviews.apache.org/r/17790/>
>>> >>> >>>>> >>> >>> >
>>> >>> >>>>> >>> >>> >a ref to ticket and fs page are in the review
>>>request.
>>> >>> >>>>> >>> >>> >
>>> >>> >>>>> >>> >>> >kind regards,
>>> >>> >>>>> >>> >>> >--
>>> >>> >>>>> >>> >>> >Daan
>>> >>> >>>>> >>> >>>
>>> >>> >>>>> >>> >>>
>>> >>> >>>>> >>> >>>
>>> >>> >>>>> >>> >>
>>> >>> >>>>> >>>
>>> >>> >>>>> >>>
>>> >>> >>>>> >>>
>>> >>> >>>>> >>> --
>>> >>> >>>>> >>> Daan
>>> >>> >>>>> >>>
>>> >>> >>>>> >>>
>>> >>> >>>>> >
>>> >>> >>>>>
>>> >>> >>>>>
>>> >>> >>>>>
>>> >>> >>>>> --
>>> >>> >>>>> Daan
>>> >>> >>>>>
>>> >>> >>>>>
>>> >>> >>>>
>>> >>> >>>
>>> >>> >>
>>> >>>
>>> >>>
>>> >>
>>>
>>>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among
Multiple Regions
Posted by Alex Ough <al...@sungardas.com>.
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 <al...@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.Prokharchyk@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 <al...@sungardas.com>
>> Date: Thursday, April 3, 2014 at 9:22 AM
>> To: Chiradeep Vittal <Ch...@citrix.com>, Alena Prokharchyk <
>> alena.prokharchyk@citrix.com>, Darren Shepherd <
>> darren.shepherd@citrix.com>
>> Cc: "dev@cloudstack.apache.org" <de...@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 <al...@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 <al...@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.Vittal@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 <Al...@citrix.com>
>>>>> Date: Wednesday, March 26, 2014 at 7:39 AM
>>>>> To: Alex Ough <al...@sungard.com>, "dev@cloudstack.apache.org" <
>>>>> dev@cloudstack.apache.org>
>>>>> Cc: Chiradeep Vittal <ch...@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 <al...@sungard.com>
>>>>> Date: Wednesday, March 26, 2014 at 6:35 AM
>>>>> To: Alena Prokharchyk <al...@citrix.com>
>>>>> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>,
>>>>> Chiradeep Vittal <Ch...@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 <al...@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.Prokharchyk@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" <al...@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.Prokharchyk@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" <al...@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.Prokharchyk@citrix.com> wrote:
>>>>>>> >> >
>>>>>>> >> >> Alex, see inline.
>>>>>>> >> >>
>>>>>>> >> >> -Alena.
>>>>>>> >> >>
>>>>>>> >> >> From: Alex Ough <al...@sungard.com>
>>>>>>> >> >> Date: Thursday, March 13, 2014 at 1:00 PM
>>>>>>> >> >> To: Alena Prokharchyk <al...@citrix.com>
>>>>>>> >> >> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>,
>>>>>>> >>Chiradeep
>>>>>>> >> >> Vittal <ch...@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.Prokharchyk@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" <al...@sungard.com>
>>>>>>> wrote:
>>>>>>> >> >>>
>>>>>>> >> >>> >Chiradeep,
>>>>>>> >> >>> >
>>>>>>> >> >>> >Any comments on them?
>>>>>>> >> >>> >
>>>>>>> >> >>> >Thanks
>>>>>>> >> >>> >Alex Ough
>>>>>>> >> >>> >
>>>>>>> >> >>> >
>>>>>>> >> >>> >On Wed, Mar 12, 2014 at 10:58 AM, Alex Ough <
>>>>>>> alex.ough@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
>>>>>>> >><al...@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
>>>>>>> >><al...@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
>>>>>>> >> >>> >>>><da...@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
>>>>>>> >> >>> >>>>> <Ch...@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.ough@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
>>>>>>> >> >>> >>>>> >><da...@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
>>>>>>> >> >>> >>>>><al...@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.ough@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.Vittal@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"
>>>>>>> >> >>> >>>>><da...@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
>>>>>>> >> >>> >>>>>
>>>>>>> >> >>> >>>>>
>>>>>>> >> >>> >>>>
>>>>>>> >> >>> >>>
>>>>>>> >> >>> >>
>>>>>>> >> >>>
>>>>>>> >> >>>
>>>>>>> >> >>
>>>>>>> >>
>>>>>>> >>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among
Multiple Regions
Posted by Alex Ough <al...@sungardas.com>.
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.Prokharchyk@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 <al...@sungardas.com>
> Date: Thursday, April 3, 2014 at 9:22 AM
> To: Chiradeep Vittal <Ch...@citrix.com>, Alena Prokharchyk <
> alena.prokharchyk@citrix.com>, Darren Shepherd <darren.shepherd@citrix.com
> >
> Cc: "dev@cloudstack.apache.org" <de...@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 <al...@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 <al...@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.Vittal@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 <Al...@citrix.com>
>>>> Date: Wednesday, March 26, 2014 at 7:39 AM
>>>> To: Alex Ough <al...@sungard.com>, "dev@cloudstack.apache.org" <
>>>> dev@cloudstack.apache.org>
>>>> Cc: Chiradeep Vittal <ch...@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 <al...@sungard.com>
>>>> Date: Wednesday, March 26, 2014 at 6:35 AM
>>>> To: Alena Prokharchyk <al...@citrix.com>
>>>> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>, Chiradeep
>>>> Vittal <Ch...@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 <al...@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.Prokharchyk@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" <al...@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.Prokharchyk@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" <al...@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.Prokharchyk@citrix.com> wrote:
>>>>>> >> >
>>>>>> >> >> Alex, see inline.
>>>>>> >> >>
>>>>>> >> >> -Alena.
>>>>>> >> >>
>>>>>> >> >> From: Alex Ough <al...@sungard.com>
>>>>>> >> >> Date: Thursday, March 13, 2014 at 1:00 PM
>>>>>> >> >> To: Alena Prokharchyk <al...@citrix.com>
>>>>>> >> >> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>,
>>>>>> >>Chiradeep
>>>>>> >> >> Vittal <ch...@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.Prokharchyk@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" <al...@sungard.com>
>>>>>> wrote:
>>>>>> >> >>>
>>>>>> >> >>> >Chiradeep,
>>>>>> >> >>> >
>>>>>> >> >>> >Any comments on them?
>>>>>> >> >>> >
>>>>>> >> >>> >Thanks
>>>>>> >> >>> >Alex Ough
>>>>>> >> >>> >
>>>>>> >> >>> >
>>>>>> >> >>> >On Wed, Mar 12, 2014 at 10:58 AM, Alex Ough <
>>>>>> alex.ough@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
>>>>>> >><al...@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
>>>>>> >><al...@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
>>>>>> >> >>> >>>><da...@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
>>>>>> >> >>> >>>>> <Ch...@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.ough@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
>>>>>> >> >>> >>>>> >><da...@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
>>>>>> >> >>> >>>>><al...@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.ough@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.Vittal@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"
>>>>>> >> >>> >>>>><da...@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
>>>>>> >> >>> >>>>>
>>>>>> >> >>> >>>>>
>>>>>> >> >>> >>>>
>>>>>> >> >>> >>>
>>>>>> >> >>> >>
>>>>>> >> >>>
>>>>>> >> >>>
>>>>>> >> >>
>>>>>> >>
>>>>>> >>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among
Multiple Regions
Posted by Alena Prokharchyk <Al...@citrix.com>.
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 <al...@sungardas.com>>
Date: Thursday, April 3, 2014 at 9:22 AM
To: Chiradeep Vittal <Ch...@citrix.com>>, Alena Prokharchyk <al...@citrix.com>>, Darren Shepherd <da...@citrix.com>>
Cc: "dev@cloudstack.apache.org<ma...@cloudstack.apache.org>" <de...@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 <al...@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 <al...@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 <Ch...@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 <Al...@citrix.com>>
Date: Wednesday, March 26, 2014 at 7:39 AM
To: Alex Ough <al...@sungard.com>>, "dev@cloudstack.apache.org<ma...@cloudstack.apache.org>" <de...@cloudstack.apache.org>>
Cc: Chiradeep Vittal <ch...@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 <al...@sungard.com>>
Date: Wednesday, March 26, 2014 at 6:35 AM
To: Alena Prokharchyk <al...@citrix.com>>
Cc: "dev@cloudstack.apache.org<ma...@cloudstack.apache.org>" <de...@cloudstack.apache.org>>, Chiradeep Vittal <Ch...@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 <al...@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 <Al...@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" <al...@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.Prokharchyk@citrix.com<ma...@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" <al...@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.Prokharchyk@citrix.com<ma...@citrix.com>> wrote:
>> >
>> >> Alex, see inline.
>> >>
>> >> -Alena.
>> >>
>> >> From: Alex Ough <al...@sungard.com>>
>> >> Date: Thursday, March 13, 2014 at 1:00 PM
>> >> To: Alena Prokharchyk <al...@citrix.com>>
>> >> Cc: "dev@cloudstack.apache.org<ma...@cloudstack.apache.org>" <de...@cloudstack.apache.org>>,
>>Chiradeep
>> >> Vittal <ch...@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.Prokharchyk@citrix.com<ma...@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" <al...@sungard.com>> wrote:
>> >>>
>> >>> >Chiradeep,
>> >>> >
>> >>> >Any comments on them?
>> >>> >
>> >>> >Thanks
>> >>> >Alex Ough
>> >>> >
>> >>> >
>> >>> >On Wed, Mar 12, 2014 at 10:58 AM, Alex Ough <al...@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
>><al...@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
>><al...@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
>> >>> >>>><da...@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
>> >>> >>>>> <Ch...@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" <al...@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
>> >>> >>>>> >><da...@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
>> >>> >>>>><al...@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
>> >>> >>>>><al...@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.Vittal@citrix.com<ma...@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"
>> >>> >>>>><da...@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
>> >>> >>>>>
>> >>> >>>>>
>> >>> >>>>
>> >>> >>>
>> >>> >>
>> >>>
>> >>>
>> >>
>>
>>
Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among
Multiple Regions
Posted by Alex Ough <al...@sungardas.com>.
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 <al...@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 <al...@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.Vittal@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 <Al...@citrix.com>
>>> Date: Wednesday, March 26, 2014 at 7:39 AM
>>> To: Alex Ough <al...@sungard.com>, "dev@cloudstack.apache.org" <
>>> dev@cloudstack.apache.org>
>>> Cc: Chiradeep Vittal <ch...@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 <al...@sungard.com>
>>> Date: Wednesday, March 26, 2014 at 6:35 AM
>>> To: Alena Prokharchyk <al...@citrix.com>
>>> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>, Chiradeep
>>> Vittal <Ch...@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 <al...@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.Prokharchyk@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" <al...@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.Prokharchyk@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" <al...@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.Prokharchyk@citrix.com> wrote:
>>>>> >> >
>>>>> >> >> Alex, see inline.
>>>>> >> >>
>>>>> >> >> -Alena.
>>>>> >> >>
>>>>> >> >> From: Alex Ough <al...@sungard.com>
>>>>> >> >> Date: Thursday, March 13, 2014 at 1:00 PM
>>>>> >> >> To: Alena Prokharchyk <al...@citrix.com>
>>>>> >> >> Cc: "dev@cloudstack.apache.org" <de...@cloudstack.apache.org>,
>>>>> >>Chiradeep
>>>>> >> >> Vittal <ch...@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.Prokharchyk@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" <al...@sungard.com> wrote:
>>>>> >> >>>
>>>>> >> >>> >Chiradeep,
>>>>> >> >>> >
>>>>> >> >>> >Any comments on them?
>>>>> >> >>> >
>>>>> >> >>> >Thanks
>>>>> >> >>> >Alex Ough
>>>>> >> >>> >
>>>>> >> >>> >
>>>>> >> >>> >On Wed, Mar 12, 2014 at 10:58 AM, Alex Ough <
>>>>> alex.ough@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
>>>>> >><al...@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
>>>>> >><al...@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
>>>>> >> >>> >>>><da...@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
>>>>> >> >>> >>>>> <Ch...@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.ough@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
>>>>> >> >>> >>>>> >><da...@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
>>>>> >> >>> >>>>><al...@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.ough@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.Vittal@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"
>>>>> >> >>> >>>>><da...@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
>>>>> >> >>> >>>>>
>>>>> >> >>> >>>>>
>>>>> >> >>> >>>>
>>>>> >> >>> >>>
>>>>> >> >>> >>
>>>>> >> >>>
>>>>> >> >>>
>>>>> >> >>
>>>>> >>
>>>>> >>
>>>>>
>>>>>
>>>>
>>>
>>
>