cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Ough <alex.o...@sungard.com>
Subject Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among Multiple Regions
Date Wed, 26 Mar 2014 15:25:34 GMT
Sure, I'm looking forward to it.

Thanks
Alex Ough


On Wed, Mar 26, 2014 at 9:39 AM, Alena Prokharchyk <
Alena.Prokharchyk@citrix.com> wrote:

>  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