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.o...@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.hoogl...@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.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
>>
>>
>
