Forgot to mention this, but it was a great help from Darren.
Thanks again, Darren!
Alex Ough
On Thu, Apr 3, 2014 at 11:56 AM, Alex Ough <alex.o...@sungardas.com> wrote:
> All,
>
> I updated the patches as per Alena's request.
>
> Let me know if there is anything missing/incorrect.
> Thanks
> Alex Ough
>
>
> On Wed, Mar 26, 2014 at 1:40 PM, Alex Ough <alex.o...@sungard.com> wrote:
>
>> Sorry, my bad. I mis-read your comment.
>>
>> Thanks for pointing it out.
>> Alex Ough
>>
>>
>> On Wed, Mar 26, 2014 at 12:25 PM, Chiradeep Vittal <
>> chiradeep.vit...@citrix.com> wrote:
>>
>>> I didn't say "do not use auto wiring". I said the convention is to use
>>> @Inject which you didn't have.
>>>
>>> From: Alena Prokharchyk <alena.prokharc...@citrix.com>
>>> Date: Wednesday, March 26, 2014 at 7:39 AM
>>> To: Alex Ough <alex.o...@sungard.com>, "dev@cloudstack.apache.org" <
>>> dev@cloudstack.apache.org>
>>> Cc: Chiradeep Vittal <chiradeep.vit...@citrix.com>
>>>
>>> Subject: Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up
>>> Among Multiple Regions
>>>
>>> Alex, I'm attending a conference today, will review the patch
>>> tomorrow.
>>>
>>> -Alena
>>>
>>> From: Alex Ough <alex.o...@sungard.com>
>>> Date: Wednesday, March 26, 2014 at 6:35 AM
>>> To: Alena Prokharchyk <alena.prokharc...@citrix.com>
>>> Cc: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Chiradeep
>>> Vittal <chiradeep.vit...@citrix.com>
>>> Subject: Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up
>>> Among Multiple Regions
>>>
>>> Alena,
>>>
>>> It took a little time to update the patch because I had a vacation
>>> last week.
>>> Now I uploaded the updated patch for review with below status, so please
>>> check them and let me know what you think.
>>> I hope it to be merged soon to wrap this up asap.
>>>
>>> 1. no change with waiting for comments on my recommendation.
>>>
>>> 2. The two things to turn on the sync-up among multiple regions is to
>>> change the class name of "eventNotificationBus" into "MultiRegionEventBus"
>>> from "RabbitMQEventBus" as below and change the value of
>>> 'region.full.scan.interval' in Global Settings. And the new classes are
>>> never used unless the feature is turned on, so I'm not sure if auto wiring
>>> is necessary here and Chiradeep asked not to use @inject in his initial
>>> review, so I removed them.
>>> <bean id="eventNotificationBus"
>>> class="org.apache.cloudstack.mom.rabbitmq.MultiRegionEventBus">
>>>
>>> 3. They are not adaptors, but inherited classes to process
>>> domain/account/user objects separately.
>>>
>>> 4. Done
>>>
>>> 5. Same
>>>
>>> 6. I removed 'domain path' from AccountResponse & UserResponse.
>>>
>>> Thanks
>>> Alex Ough
>>>
>>>
>>> On Thu, Mar 13, 2014 at 9:48 PM, Alex Ough <alex.o...@sungard.com>wrote:
>>>
>>>> What I think based on your comments are
>>>>
>>>> 1. Well, I have a different thought. I'd rather have separated
>>>> classes instead of having a class with lots of methods, which makes the
>>>> maintenance easier. And as you show as an example, it is possible to create
>>>> a method and merge them, but I think it is better to provide separate
>>>> methods that are exposed outside so that the callers can know what to call
>>>> with ease.
>>>>
>>>> 2. Let me look at that.
>>>>
>>>> 3. I'm not sure why you think they are adapters, but let me look at
>>>> that class.
>>>>
>>>> 4. OK, let me work on this.
>>>>
>>>> 5. The urls are stored in the region table along with username and
>>>> password. Please see 'RegionVO' under
>>>> 'engine/schema/src/org/apache/cloudstack/region'.
>>>>
>>>> Thanks
>>>> Alex Ough
>>>>
>>>>
>>>> On Thu, Mar 13, 2014 at 5:49 PM, Alena Prokharchyk <
>>>> alena.prokharc...@citrix.com> wrote:
>>>>
>>>>> Alex,
>>>>>
>>>>> There are so many classes, and it makes it hard to see/review the
>>>>> feature.
>>>>> Can you come up with some sort of visual diagram, so its easier to see
>>>>> which component is responsible for what task, and how they interact
>>>>> with
>>>>> each other.
>>>>>
>>>>> My suggestions:
>>>>>
>>>>> 1) I think it would make sense to merge all the classes doing utils
>>>>> tasks
>>>>> (forming and sending Apis to CS) - UserInterface, AccountInterface,
>>>>> DomainInterface - to a single util class. They do return generic types
>>>>> anyway - JsonArray/JsonObject, and they do perform a generic util task
>>>>> -
>>>>> forming and sending the request to the CS. I would merge them all with
>>>>> the
>>>>> BaseInterface and name it with the name indicating that this class is
>>>>> responsible for sending API calls against CS. And this would be your
>>>>> util
>>>>> class.
>>>>>
>>>>>
>>>>> You should also come up with some generic method that forms the
>>>>> requests
>>>>> to CS APIs to make the code cleaner.
>>>>>
>>>>> For example, look at these 2:
>>>>>
>>>>>
>>>>> public JSONObject lockUser(String userId) throws Exception {
>>>>> String paramStr = "command=lockUser&id=" + userId +
>>>>> "&response=json&sessionkey=" + URLEncoder.encode(getSessionKey(),
>>>>> "UTF-8");
>>>>> return sendApacheGet(paramStr);
>>>>> }
>>>>>
>>>>>
>>>>> public JSONObject disableUser(String userId) throws Exception {
>>>>>
>>>>> String paramStr = "command=disableUser&id=" + userId +
>>>>> "&response=json&sessionkey=" + URLEncoder.encode(getSessionKey(),
>>>>> "UTF-8");
>>>>> return sendApacheGet(paramStr);
>>>>> }
>>>>>
>>>>>
>>>>> You repeat appending json and session key in all of them. Why not
>>>>> create
>>>>> some generic method where you pass a) commandName 2) map of parameters,
>>>>> and make that method return JsonObject/JsonArray?
>>>>>
>>>>>
>>>>> 2) I would suggest you utilize Spring framework in your code and auto
>>>>> wire
>>>>> all the dependencies by using @Inject rather than locating them with
>>>>> the
>>>>> components lifecycle. Please refer to Apache Wiki:
>>>>>
>>>>>
>>>>> https://cwiki.apache.org/confluence/display/CLOUDSTACK/Using+Spring+in+Clou
>>>>> dStack
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> 3) AccountFullSyncProcessor/DomainFullSyncProcessor/<other processors>.
>>>>> These looks like adaptors to me. Have you looked at how AdapterBase is
>>>>> utilized in CS? You should take a look at it.
>>>>>
>>>>>
>>>>> 4) I see that you have a folder called "simulator". Does this folder
>>>>> contain Test classes used by some kind of simulator? Or would they be
>>>>> used
>>>>> in production? If its just for testing, please rename them by putting
>>>>> the
>>>>> word "Simulator" in the name. Look at how other simulator classes are
>>>>> named in CS (SimulatorImageStoreLifeCycleImpl.java for example)
>>>>>
>>>>> 5) In the code, I haven't noticed where you store/read the end point
>>>>> URL
>>>>> to the CS Management server - the server address you are gonna send
>>>>> your
>>>>> requests to. If for example, you have MS1 and MS2, will your plugin
>>>>> from
>>>>> MS1 ever sends a request to MS2? And if it sends the request only to
>>>>> MS1,
>>>>> what ip address it uses?
>>>>>
>>>>>
>>>>>
>>>>> -Alena.
>>>>>
>>>>> On 3/13/14, 2:58 PM, "Alex Ough" <alex.o...@sungard.com> wrote:
>>>>>
>>>>> >They are not called outside and only called from 'subscriber' classes
>>>>> and
>>>>> >FullScanner class.
>>>>> >
>>>>> >Do you think these name changes are ok?
>>>>> >
>>>>> > - BaseInterface - UserInterface, AccountInterface, DomainInterface
>>>>> > => APICaller - APIUserCaller, APIAccountCaller, APIDomainCaller
>>>>> >
>>>>> > - BaseService - UserService, AccountService, DomainService
>>>>> > => RemoteResourceProcessor - RemoteUserProcessor,
>>>>> >RemoteAccountProcessor, RemoteDomainProcessor
>>>>> >
>>>>> > - FullSyncProcessor - UserFullSyncProcessor,
>>>>> AccountFullSyncProcessor,
>>>>> >DomainFullSyncProcessor
>>>>> > => no change
>>>>> >
>>>>> > - RemoteEventProcessor - RemoteUserEventProcessor,
>>>>> >RemoteAccountEventProcessor, RemoteDomainEventProcessor
>>>>> > => no change
>>>>> >
>>>>> >Thanks
>>>>> >Alex Ough
>>>>> >
>>>>> >
>>>>> >
>>>>> >On Thu, Mar 13, 2014 at 3:58 PM, Alena Prokharchyk <
>>>>> >alena.prokharc...@citrix.com> wrote:
>>>>> >
>>>>> >> You extract stuff into interfaces when the methods are meant to be
>>>>> >>called
>>>>> >> from different classes/Managers. Do you implement to add APIs for
>>>>> your
>>>>> >> plugins? Can your plugin be used by any other CS manager -
>>>>> RegionManager
>>>>> >> for example? If the answer is yes, then you would need an
>>>>> interface. If
>>>>> >> not, abstract class is fine, just remove Interface/Service from the
>>>>> >>name.
>>>>> >> Also rename your classes to reflect the purpose they are developed
>>>>> for;
>>>>> >> account/user/domain is way too generic and already used in other CS
>>>>> >> packages.
>>>>> >>
>>>>> >>
>>>>> >>
>>>>> >>
>>>>> >>
>>>>> >> On 3/13/14, 1:50 PM, "Alex Ough" <alex.o...@sungard.com> wrote:
>>>>> >>
>>>>> >> >Patch B,
>>>>> >> >
>>>>> >> >1. The reason why I use abstract classes instead of interfaces is
>>>>> >>because
>>>>> >> >there are some basic methods that are used among the inherited
>>>>> >>classes, so
>>>>> >> >I'm not sure why it has to be an interface.
>>>>> >> >
>>>>> >> >2. These are the abstract base classes along with their inherited
>>>>> >>classes
>>>>> >> >and they are grouped by their behavior.
>>>>> >> > - BaseInterface - UserInterface, AccountInterface,
>>>>> DomainInterface
>>>>> >> > - BaseService - UserService, AccountService, DomainService
>>>>> >> > - FullSyncProcessor - UserFullSyncProcessor,
>>>>> >>AccountFullSyncProcessor,
>>>>> >> >DomainFullSyncProcessor
>>>>> >> > - RemoteEventProcessor - RemoteUserEventProcessor,
>>>>> >> >RemoteAccountEventProcessor, RemoteDomainEventProcessor
>>>>> >> >
>>>>> >> > => Do you recommend to refactor them into defining interfaces
>>>>> and
>>>>> >> >creating one class implementing all methods related with each user,
>>>>> >> >account
>>>>> >> >and domain?
>>>>> >> >
>>>>> >> >3. Let me work on changing names.
>>>>> >> >
>>>>> >> >Thanks
>>>>> >> >Alex Ough
>>>>> >> >
>>>>> >> >
>>>>> >> >
>>>>> >> >
>>>>> >> >On Thu, Mar 13, 2014 at 3:14 PM, Alena Prokharchyk <
>>>>> >> >alena.prokharc...@citrix.com> wrote:
>>>>> >> >
>>>>> >> >> Alex, see inline.
>>>>> >> >>
>>>>> >> >> -Alena.
>>>>> >> >>
>>>>> >> >> From: Alex Ough <alex.o...@sungard.com>
>>>>> >> >> Date: Thursday, March 13, 2014 at 1:00 PM
>>>>> >> >> To: Alena Prokharchyk <alena.prokharc...@citrix.com>
>>>>> >> >> Cc: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>,
>>>>> >>Chiradeep
>>>>> >> >> Vittal <chirade...@gmail.com>
>>>>> >> >>
>>>>> >> >> Subject: Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User
>>>>> Sync Up
>>>>> >> >> Among Multiple Regions
>>>>> >> >>
>>>>> >> >> Hi Alena,
>>>>> >> >>
>>>>> >> >> Patch B,
>>>>> >> >> I'm not quite familiar with java, so I have a little difficulty
>>>>> in
>>>>> >> >> following your recommendation.
>>>>> >> >> Can you send me an example using 'BaseInterface' and/or
>>>>> >> >>'AccountInterface'?
>>>>> >> >>
>>>>> >> >>
>>>>> >> >> - What is called an interface in java:
>>>>> >> >>
>>>>> >>http://docs.oracle.com/javase/tutorial/java/concepts/interface.html.
>>>>> >> >> Its a place where all your methods are defined w/o actual
>>>>> >> >>implementation.
>>>>> >> >> - Look at any of cloudStack Managers implementation. Take for
>>>>> >> >>example:
>>>>> >> >>
>>>>> >> >>
>>>>> >> >> 1. AcccountManagerImpl.java - class where all the methods are
>>>>> >> >> implemented. Part of the server package
>>>>> >> >> 2. AccountManagerImpl implements 2 interfaces -
>>>>> AccountManager and
>>>>> >> >> AccountService. If you want any of your methods to be used by
>>>>> >> >> plugins/services, define them in Service interface. If all of
>>>>> them
>>>>> >> >>are
>>>>> >> >> meant to be used just inside your plugin/or cloudstack
>>>>> >>core/server -
>>>>> >> >>define
>>>>> >> >> them in Manager interface.
>>>>> >> >> 3. I would suggest you rename your classes/interfaces by
>>>>> adding
>>>>> >>your
>>>>> >> >> feature specific keyword to the name. CloudStack already has
>>>>> >> >>AccountService
>>>>> >> >> interface. And BaseInterface name is way too generic. Plus you
>>>>> >> >>shouldn't
>>>>> >> >> really put an "Interface" to the name.
>>>>> >> >>
>>>>> >> >>
>>>>> >> >>
>>>>> >> >> It will be very helpful and appreciated.
>>>>> >> >>
>>>>> >> >> Patch A,
>>>>> >> >> To reduce the number of requests to the remote regions
>>>>> >> >> because the syncing is always using the api requests a lot to get
>>>>> >> >> information of domains/accounts/users from remote regions.
>>>>> >> >>
>>>>> >> >>
>>>>> >> >>
>>>>> >> >> - you can't ,modify cloudStack core/server code only to fix
>>>>> >>problem
>>>>> >> >> that is specific to your plugin/service. The solution for your
>>>>> >>case
>>>>> >> >>will be
>>>>> >> >> - create in memory data structure where you keep
>>>>> account/domain
>>>>> >> >> information. Account->domain relationship don't change along
>>>>> >>account
>>>>> >> >> lifecycle, as well as the domain path doesn't change for the
>>>>> >>domain
>>>>> >> >>once
>>>>> >> >> its created. And get the domain path from there.
>>>>> >> >>
>>>>> >> >>
>>>>> >> >>
>>>>> >> >> And let me change the concatenation into using StringBuilder.
>>>>> >> >>
>>>>> >> >> Thanks a lot for your reply.
>>>>> >> >> Alex Ough
>>>>> >> >>
>>>>> >> >>
>>>>> >> >> On Thu, Mar 13, 2014 at 2:41 PM, Alena Prokharchyk <
>>>>> >> >> alena.prokharc...@citrix.com> wrote:
>>>>> >> >>
>>>>> >> >>> Alex, I have some comments.
>>>>> >> >>>
>>>>> >> >>> Patch B.
>>>>> >> >>>
>>>>> >> >>> * You shouldn't make your service a part of cloud-mom-rabbitmq
>>>>> >>plugin.
>>>>> >> >>> Your subscribers/implementation are specific to your feature,
>>>>> and
>>>>> >>you
>>>>> >> >>>need
>>>>> >> >>> to introduce a special plugin just for your service.
>>>>> >> >>> * AccountInterface and BaseInterface are still regular classes.
>>>>> You
>>>>> >> >>>should
>>>>> >> >>> split them into Service interface /ManagerImpl or Manager
>>>>> interface
>>>>> >> >>> /ManagerImpl as Chiradeep suggested.
>>>>> >> >>> * Once you extract services interfaces, make sure you don't use
>>>>> VO
>>>>> >> >>>objects
>>>>> >> >>> in methods signatures.
>>>>> >> >>> * You should really get a use of @Manager interface and @Inject
>>>>> >> >>> annotations for autowiring your managers instead of setting
>>>>> them up
>>>>> >> >>>using
>>>>> >> >>> ComponentContext.getComponent() in each of your manager classes.
>>>>> >> >>>
>>>>> >> >>>
>>>>> >> >>>
>>>>> >> >>> Patch A.
>>>>> >> >>>
>>>>> >> >>> * AccountResponse. Why did you add domain path to the account
>>>>> >>response?
>>>>> >> >>> You can always retrieve it by a) get domain info from account
>>>>> >>response
>>>>> >> >>>b)
>>>>> >> >>> execute listDomains&domainId to get the domain path. Try not to
>>>>> >> >>>overload
>>>>> >> >>> the response with attributes that don't belong to response's
>>>>> first
>>>>> >> >>>class
>>>>> >> >>> object.
>>>>> >> >>>
>>>>> >> >>>
>>>>> >> >>> Generic comments.
>>>>> >> >>>
>>>>> >> >>> * I can see that you do a lot of string concatenation this way:
>>>>> >> >>>paramStr
>>>>> >> >>> += "&email=" + email + "&firstname=" + firstName + "&lastname="
>>>>> +
>>>>> >> >>>lastName
>>>>> >> >>> + "&accounttype=" + accountType;
>>>>> >> >>> I would suggest to use StringBuilder instead.
>>>>> >> >>>
>>>>> >> >>>
>>>>> >> >>>
>>>>> >> >>>
>>>>> >> >>> On 3/13/14, 9:33 AM, "Alex Ough" <alex.o...@sungard.com> wrote:
>>>>> >> >>>
>>>>> >> >>> >Chiradeep,
>>>>> >> >>> >
>>>>> >> >>> >Any comments on them?
>>>>> >> >>> >
>>>>> >> >>> >Thanks
>>>>> >> >>> >Alex Ough
>>>>> >> >>> >
>>>>> >> >>> >
>>>>> >> >>> >On Wed, Mar 12, 2014 at 10:58 AM, Alex Ough <
>>>>> alex.o...@sungard.com>
>>>>> >> >>> wrote:
>>>>> >> >>> >
>>>>> >> >>> >> And I also uploaded the patch B that includes new
>>>>> implementation
>>>>> >>to
>>>>> >> >>> >> support multi regions.
>>>>> >> >>> >>
>>>>> >> >>> >> Thanks
>>>>> >> >>> >> Alex Ough
>>>>> >> >>> >>
>>>>> >> >>> >>
>>>>> >> >>> >> On Wed, Mar 12, 2014 at 10:17 AM, Alex Ough
>>>>> >><alex.o...@sungard.com>
>>>>> >> >>> >>wrote:
>>>>> >> >>> >>
>>>>> >> >>> >>> I uploaded the patch A that includes only core changes, so
>>>>> >>please
>>>>> >> >>> >>>review
>>>>> >> >>> >>> it and let me know if you have any comments.
>>>>> >> >>> >>>
>>>>> >> >>> >>> Thanks
>>>>> >> >>> >>> Alex Ough
>>>>> >> >>> >>>
>>>>> >> >>> >>>
>>>>> >> >>> >>> On Wed, Mar 12, 2014 at 8:24 AM, Alex Ough
>>>>> >><alex.o...@sungard.com>
>>>>> >> >>> >>>wrote:
>>>>> >> >>> >>>
>>>>> >> >>> >>>> Hi Chiradeep,
>>>>> >> >>> >>>>
>>>>> >> >>> >>>> Can you give me the example of the Singleton service class
>>>>> you
>>>>> >> >>> >>>>mentioned?
>>>>> >> >>> >>>> I'm not sure if you are asking the name changes or else
>>>>> because
>>>>> >> >>>those
>>>>> >> >>> >>>> classes are abstract classes and do not need to be
>>>>> singleton
>>>>> >> >>>class.
>>>>> >> >>> >>>>
>>>>> >> >>> >>>> And let me try the refactoring and ask confirmations to
>>>>> you,
>>>>> >>so I
>>>>> >> >>> >>>>hope I
>>>>> >> >>> >>>> can get the reply soon.
>>>>> >> >>> >>>>
>>>>> >> >>> >>>> Thanks
>>>>> >> >>> >>>> Alex Ough
>>>>> >> >>> >>>>
>>>>> >> >>> >>>>
>>>>> >> >>> >>>> On Wed, Mar 12, 2014 at 4:13 AM, Daan Hoogland
>>>>> >> >>> >>>><daan.hoogl...@gmail.com>wrote:
>>>>> >> >>> >>>>
>>>>> >> >>> >>>>> H Alex, I considder Chiradeeps comments quite valid and
>>>>> >>serious
>>>>> >> >>> >>>>>enough
>>>>> >> >>> >>>>> to anticipate not making friday 14:00 CET. That would
>>>>> mean no
>>>>> >> >>>merge
>>>>> >> >>> >>>>> before 4.4. Can you live with that?
>>>>> >> >>> >>>>>
>>>>> >> >>> >>>>> On Wed, Mar 12, 2014 at 6:40 AM, Chiradeep Vittal
>>>>> >> >>> >>>>> <chiradeep.vit...@citrix.com> wrote:
>>>>> >> >>> >>>>> > Hi Alex,
>>>>> >> >>> >>>>> >
>>>>> >> >>> >>>>> > If you look at the general design of CloudStack, there
>>>>> are
>>>>> >> >>> >>>>>Singleton
>>>>> >> >>> >>>>> > service interfaces which are then implemented with real
>>>>> >> >>>classes.
>>>>> >> >>> >>>>>This
>>>>> >> >>> >>>>> > facilitates easy testing by mocking the interface. In
>>>>> your
>>>>> >>new
>>>>> >> >>> >>>>>files
>>>>> >> >>> >>>>> > (BaseInterface, which by the way is NOT an interface,
>>>>> >> >>> >>>>>AccountService,
>>>>> >> >>> >>>>> > which is NOT a service like other CloudStack services,
>>>>> and
>>>>> >>so
>>>>> >> >>>on),
>>>>> >> >>> >>>>> this
>>>>> >> >>> >>>>> > design paradigm is not followed. Therefore it is
>>>>> >>incongruous to
>>>>> >> >>> the
>>>>> >> >>> >>>>> rest
>>>>> >> >>> >>>>> > of the code base.
>>>>> >> >>> >>>>> >
>>>>> >> >>> >>>>> > Furthermore this has been plopped right in the middle of
>>>>> >>other
>>>>> >> >>> core
>>>>> >> >>> >>>>> > CloudStack code (server/src/com/cloud/region/). If you
>>>>> look
>>>>> >>at
>>>>> >> >>>the
>>>>> >> >>> >>>>> > EventBus logic, it has been written as a plugin, thereby
>>>>> >> >>>enabling
>>>>> >> >>> >>>>> users
>>>>> >> >>> >>>>> > who do not need it to disable it. This level of
>>>>> >>configuration
>>>>> >> >>>is
>>>>> >> >>> >>>>> needed
>>>>> >> >>> >>>>> > and I can't find it.
>>>>> >> >>> >>>>> >
>>>>> >> >>> >>>>> > So, to summarize:
>>>>> >> >>> >>>>> > 1. Split it into patches. Patch A is the change to the
>>>>> core
>>>>> >> >>>DAOs,
>>>>> >> >>> >>>>>db
>>>>> >> >>> >>>>> > logic, schema upgrade code, etc.
>>>>> >> >>> >>>>> > 2. Patch B is the actual sync service written as an
>>>>> optional
>>>>> >> >>> plugin
>>>>> >> >>> >>>>> with
>>>>> >> >>> >>>>> > the package name space of org.apache.
>>>>> >> >>> >>>>> >
>>>>> >> >>> >>>>> > On 3/11/14, 3:09 PM, "Alex Ough" <alex.o...@sungard.com
>>>>> >
>>>>> >> wrote:
>>>>> >> >>> >>>>> >
>>>>> >> >>> >>>>> >>Hi Daan,
>>>>> >> >>> >>>>> >>
>>>>> >> >>> >>>>> >>I didn't update the patch after the couple of works
>>>>> because
>>>>> >>I
>>>>> >> >>> >>>>>wanted
>>>>> >> >>> >>>>> to do
>>>>> >> >>> >>>>> >>it with others Chiradeep asked if any.
>>>>> >> >>> >>>>> >>Let me know when you want me to.
>>>>> >> >>> >>>>> >>
>>>>> >> >>> >>>>> >>Thanks
>>>>> >> >>> >>>>> >>Alex Ough
>>>>> >> >>> >>>>> >>
>>>>> >> >>> >>>>> >>
>>>>> >> >>> >>>>> >>On Tue, Mar 11, 2014 at 4:37 PM, Daan Hoogland
>>>>> >> >>> >>>>> >><daan.hoogl...@gmail.com>wrote:
>>>>> >> >>> >>>>> >>
>>>>> >> >>> >>>>> >>> I will call the merge in a moment, but won't do it
>>>>> >> >>>immediately.
>>>>> >> >>> >>>>> >>>
>>>>> >> >>> >>>>> >>> Be there for me when any issues come up. If not I will
>>>>> >>revert
>>>>> >> >>> it.
>>>>> >> >>> >>>>> >>>
>>>>> >> >>> >>>>> >>> @Chiradeep: did Alex answer your concerns adequately?
>>>>> >> >>> >>>>> >>>
>>>>> >> >>> >>>>> >>>
>>>>> >> >>> >>>>> >>> kind regards,
>>>>> >> >>> >>>>> >>>
>>>>> >> >>> >>>>> >>> On Tue, Mar 11, 2014 at 6:43 PM, Alex Ough
>>>>> >> >>> >>>>><alex.o...@sungard.com>
>>>>> >> >>> >>>>> >>>wrote:
>>>>> >> >>> >>>>> >>> > I worked on a couple of items, so can you give me
>>>>> the
>>>>> >> >>> >>>>> confirmation for
>>>>> >> >>> >>>>> >>> the
>>>>> >> >>> >>>>> >>> > rest items so that I can wrap this up?
>>>>> >> >>> >>>>> >>> > I really want to include this into 4.4.
>>>>> >> >>> >>>>> >>> >
>>>>> >> >>> >>>>> >>> > Thanks
>>>>> >> >>> >>>>> >>> > Alex Ough
>>>>> >> >>> >>>>> >>> >
>>>>> >> >>> >>>>> >>> >
>>>>> >> >>> >>>>> >>> > On Mon, Mar 10, 2014 at 3:41 PM, Alex Ough
>>>>> >> >>> >>>>><alex.o...@sungard.com
>>>>> >> >>> >>>>> >
>>>>> >> >>> >>>>> >>> wrote:
>>>>> >> >>> >>>>> >>> >
>>>>> >> >>> >>>>> >>> >> Please see my reply/question in blue.
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >> Thanks
>>>>> >> >>> >>>>> >>> >> Alex Ough
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >> On Mon, Mar 10, 2014 at 1:25 PM, Chiradeep Vittal <
>>>>> >> >>> >>>>> >>> >> chiradeep.vit...@citrix.com> wrote:
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>> I haven¹t looked at it because it is huge. But
>>>>> >> >>>preliminary
>>>>> >> >>> >>>>>scan:
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >>> - there are unit tests missing for changes to
>>>>> core CS
>>>>> >> >>>code
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >> Unit tests for only new classes were added
>>>>> >> >>>because I
>>>>> >> >>> >>>>> >>>couldn't
>>>>> >> >>> >>>>> >>> >> find unit tests of the ones I modified
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>> - uses com.amazonaws.util.json (why?)
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >> They are used to handle the json objects.
>>>>> Let
>>>>> >>me
>>>>> >> >>> know
>>>>> >> >>> >>>>> if you
>>>>> >> >>> >>>>> >>> >> want me to replace it with another.
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >> - code format does not follow coding convention (
>>>>> >> >>>placement
>>>>> >> >>> of
>>>>> >> >>> >>>>> {},
>>>>> >> >>> >>>>> >>>camel
>>>>> >> >>> >>>>> >>> >>> case api_interface )
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >> Done
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>> - package namespace is com.cloud instead of
>>>>> org.apache
>>>>> >> >>>for
>>>>> >> >>> >>>>>new
>>>>> >> >>> >>>>> files
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >> I didn't know that. So do I need to use
>>>>> >> >>>'org.apache'
>>>>> >> >>> >>>>> package
>>>>> >> >>> >>>>> >>> for
>>>>> >> >>> >>>>> >>> >> all new classes?
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >> - no file-level comments. What does
>>>>> LocalAccountManager
>>>>> >> >>>do?
>>>>> >> >>> >>>>>Why
>>>>> >> >>> >>>>> does
>>>>> >> >>> >>>>> >>>it
>>>>> >> >>> >>>>> >>> >>> exist? Etc.
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >> Done.
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>> - No interfaces, only abstract classes. No use of
>>>>> >> >>>@Inject.
>>>>> >> >>> >>>>>While
>>>>> >> >>> >>>>> >>>this
>>>>> >> >>> >>>>> >>> >>> might be OK, it does make it harder to test and
>>>>> does
>>>>> >>not
>>>>> >> >>> >>>>>follow
>>>>> >> >>> >>>>> the
>>>>> >> >>> >>>>> >>> rest
>>>>> >> >>> >>>>> >>> >>> of ACS conventions.
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >> I don't think there is any interface, but
>>>>> let
>>>>> >>me
>>>>> >> >>>know
>>>>> >> >>> >>>>>if
>>>>> >> >>> >>>>> you
>>>>> >> >>> >>>>> >>> find
>>>>> >> >>> >>>>> >>> >> any.
>>>>> >> >>> >>>>> >>> >> Any recommendation to replace @inject?
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >>> I would urge the submitter to break up the
>>>>> submission.
>>>>> >> >>> >>>>> >>> >>> A) the changes to CS core, with explanations /
>>>>> >>comments
>>>>> >> >>>and
>>>>> >> >>> >>>>> tests
>>>>> >> >>> >>>>> >>> >>> B) the sync service should be an interface with
>>>>> >>concrete
>>>>> >> >>> >>>>> >>> implementations
>>>>> >> >>> >>>>> >>> >>> in its own package
>>>>> >> >>> >>>>> >>> >>> C) more tests
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >> can you give me a little specific what
>>>>> kind of
>>>>> >> >>>tests
>>>>> >> >>> >>>>>you
>>>>> >> >>> >>>>> need
>>>>> >> >>> >>>>> >>> >> more?
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>> >>> On 3/10/14, 3:37 AM, "Daan Hoogland"
>>>>> >> >>> >>>>><daan.hoogl...@gmail.com>
>>>>> >> >>> >>>>> >>>wrote:
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >>> >Hi everyody,
>>>>> >> >>> >>>>> >>> >>> >
>>>>> >> >>> >>>>> >>> >>> >The people at sungard have been making this
>>>>> change
>>>>> >>for
>>>>> >> >>>4.4
>>>>> >> >>> >>>>>and
>>>>> >> >>> >>>>> I
>>>>> >> >>> >>>>> >>>want
>>>>> >> >>> >>>>> >>> >>> >to merge/apply it this week. It is more then a
>>>>> >> >>>screenfull
>>>>> >> >>> >>>>>and
>>>>> >> >>> >>>>> might
>>>>> >> >>> >>>>> >>> >>> >cause issue is a setup or two.
>>>>> >> >>> >>>>> >>> >>> >
>>>>> >> >>> >>>>> >>> >>> >have a look at
>>>>> https://reviews.apache.org/r/17790/
>>>>> >> >>> >>>>> >>> >>> >
>>>>> >> >>> >>>>> >>> >>> >a ref to ticket and fs page are in the review
>>>>> >>request.
>>>>> >> >>> >>>>> >>> >>> >
>>>>> >> >>> >>>>> >>> >>> >kind regards,
>>>>> >> >>> >>>>> >>> >>> >--
>>>>> >> >>> >>>>> >>> >>> >Daan
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >>>
>>>>> >> >>> >>>>> >>> >>
>>>>> >> >>> >>>>> >>>
>>>>> >> >>> >>>>> >>>
>>>>> >> >>> >>>>> >>>
>>>>> >> >>> >>>>> >>> --
>>>>> >> >>> >>>>> >>> Daan
>>>>> >> >>> >>>>> >>>
>>>>> >> >>> >>>>> >>>
>>>>> >> >>> >>>>> >
>>>>> >> >>> >>>>>
>>>>> >> >>> >>>>>
>>>>> >> >>> >>>>>
>>>>> >> >>> >>>>> --
>>>>> >> >>> >>>>> Daan
>>>>> >> >>> >>>>>
>>>>> >> >>> >>>>>
>>>>> >> >>> >>>>
>>>>> >> >>> >>>
>>>>> >> >>> >>
>>>>> >> >>>
>>>>> >> >>>
>>>>> >> >>
>>>>> >>
>>>>> >>
>>>>>
>>>>>
>>>>
>>>
>>
>
