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