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
