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> 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/
>>> >>> >
>>> >>> >a ref to ticket and fs page are in the review request.
>>> >>> >
>>> >>> >kind regards,
>>> >>> >--
>>> >>> >Daan
>>> >>>
>>> >>>
>>> >>>
>>> >>
>>>
>>>
>>>
>>> --
>>> Daan
>>>
>>>
>
--
Daan
