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