Chiradeep,
Any comments on them?
Thanks
Alex Ough
On Wed, Mar 12, 2014 at 10:58 AM, Alex Ough <alex.o...@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.o...@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.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
>>>>
>>>>
>>>
>>
>
