cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chiradeep Vittal <Chiradeep.Vit...@citrix.com>
Subject Re: [DISCUSS]{BEHAVIORAL-CHANGE]Domain-Account-User Sync Up Among Multiple Regions
Date Mon, 10 Mar 2014 18:25:47 GMT
I havenĀ¹t looked at it because it is huge. But preliminary scan:

- there are unit tests missing for changes to core CS code
- uses com.amazonaws.util.json (why?)
- code format does not follow coding convention ( placement of {}, camel
case api_interface )
- package namespace is com.cloud instead of org.apache for new files
- no file-level comments. What does LocalAccountManager do? Why does it
exist? Etc.
- 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 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

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


Mime
View raw message