cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daan Hoogland <daan.hoogl...@gmail.com>
Subject Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among Multiple Regions
Date Wed, 12 Mar 2014 09:13:46 GMT
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
View raw message