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
>>
>>
