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