cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Ough <alex.o...@sungardas.com>
Subject Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among Multiple Regions
Date Thu, 03 Apr 2014 15:56:31 GMT
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.ough@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 <Alena.Prokharchyk@citrix.com>
>> Date: Wednesday, March 26, 2014 at 7:39 AM
>> To: Alex Ough <alex.ough@sungard.com>, "dev@cloudstack.apache.org" <
>> dev@cloudstack.apache.org>
>> Cc: Chiradeep Vittal <chiradeep.vittal@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.ough@sungard.com>
>> Date: Wednesday, March 26, 2014 at 6:35 AM
>> To: Alena Prokharchyk <alena.prokharchyk@citrix.com>
>> Cc: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>, Chiradeep
>> Vittal <Chiradeep.Vittal@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.ough@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" <alex.ough@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" <alex.ough@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 <alex.ough@sungard.com>
>>>> >> >> Date: Thursday, March 13, 2014 at 1:00 PM
>>>> >> >> To: Alena Prokharchyk <alena.prokharchyk@citrix.com>
>>>> >> >> Cc: "dev@cloudstack.apache.org" <dev@cloudstack.apache.org>,
>>>> >>Chiradeep
>>>> >> >> Vittal <chiradeepv@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" <alex.ough@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
>>>> >><alex.ough@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.ough@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.hoogland@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.Vittal@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
>>>> >> >>> >>>>> >><daan.hoogland@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.ough@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"
>>>> >> >>> >>>>><daan.hoogland@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
>>>> >> >>> >>>>>
>>>> >> >>> >>>>>
>>>> >> >>> >>>>
>>>> >> >>> >>>
>>>> >> >>> >>
>>>> >> >>>
>>>> >> >>>
>>>> >> >>
>>>> >>
>>>> >>
>>>>
>>>>
>>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message