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 )
>
Let me work on this.
> - 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.
>
Let me work on this
> - 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
>
>
>
