Alex, I have some comments.
Patch B.
* You shouldn’t make your service a part of cloud-mom-rabbitmq plugin.
Your subscribers/implementation are specific to your feature, and you need
to introduce a special plugin just for your service.
* AccountInterface and BaseInterface are still regular classes. You should
split them into Service interface /ManagerImpl or Manager interface
/ManagerImpl as Chiradeep suggested.
* Once you extract services interfaces, make sure you don’t use VO objects
in methods signatures.
* You should really get a use of @Manager interface and @Inject
annotations for autowiring your managers instead of setting them up using
ComponentContext.getComponent() in each of your manager classes.
Patch A.
* AccountResponse. Why did you add domain path to the account response?
You can always retrieve it by a) get domain info from account response b)
execute listDomains&domainId to get the domain path. Try not to overload
the response with attributes that don’t belong to response’s first class
object.
Generic comments.
* I can see that you do a lot of string concatenation this way: paramStr
+= "&email=" + email + "&firstname=" + firstName + "&lastname=" + lastName
+ "&accounttype=" + accountType;
I would suggest to use StringBuilder instead.
On 3/13/14, 9:33 AM, "Alex Ough" <alex.o...@sungard.com> wrote:
>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
>>>>>
>>>>>
>>>>
>>>
>>
