cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chiradeep Vittal <Chiradeep.Vit...@citrix.com>
Subject Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among Multiple Regions
Date Wed, 26 Mar 2014 17:25:22 GMT
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<mailto:Alena.Prokharchyk@citrix.com>>
Date: Wednesday, March 26, 2014 at 7:39 AM
To: Alex Ough <alex.ough@sungard.com<mailto:alex.ough@sungard.com>>, "dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>"
<dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>>
Cc: Chiradeep Vittal <chiradeep.vittal@citrix.com<mailto: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<mailto:alex.ough@sungard.com>>
Date: Wednesday, March 26, 2014 at 6:35 AM
To: Alena Prokharchyk <alena.prokharchyk@citrix.com<mailto:alena.prokharchyk@citrix.com>>
Cc: "dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>" <dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>>,
Chiradeep Vittal <Chiradeep.Vittal@citrix.com<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto:Alena.Prokharchyk@citrix.com>> wrote:
>> >
>> >>  Alex, see inline.
>> >>
>> >>  -Alena.
>> >>
>> >>   From: Alex Ough <alex.ough@sungard.com<mailto:alex.ough@sungard.com>>
>> >> Date: Thursday, March 13, 2014 at 1:00 PM
>> >> To: Alena Prokharchyk <alena.prokharchyk@citrix.com<mailto:alena.prokharchyk@citrix.com>>
>> >> Cc: "dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>"
<dev@cloudstack.apache.org<mailto:dev@cloudstack.apache.org>>,
>>Chiradeep
>> >> Vittal <chiradeepv@gmail.com<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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<mailto: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