cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Ough <alex.o...@sungard.com>
Subject Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among Multiple Regions
Date Tue, 11 Mar 2014 22:09:11 GMT
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.hoogland@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.ough@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.ough@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.Vittal@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.hoogland@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
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message