+1, but we need to agree on the target release for this change.
Andreas
On Mon, May 11, 2009 at 09:57, keith chapman <keithgchap...@gmail.com> wrote:
> Shall we go ahead with this change then?
>
> Thanks,
> Keith.
>
> On Wed, May 6, 2009 at 7:11 PM, Amila Suriarachchi
> <amilasuriarach...@gmail.com> wrote:
>>
>>
>> On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <g...@thoughtcraft.com>
>> wrote:
>>>
>>> Amila Suriarachchi wrote:
>>> > Hm - it seems like you didn't actually get my point. We CAN'T
>>> > return the
>>> > actual allServices map because that would be breaking the
>>> > abstraction
>>> > boundary provided by the class.
>>> >
>>> > As I remember this change was done to avoid concurrent modifications to
>>> > service map[1].
>>>
>>> Right - before this change we were doing something actively bad/wrong,
>>> i.e.
>>> returning the internal map. After the change we were returning a cloned
>>> copy
>>> of the map (though not using clone() for some reason), which is good in
>>> that
>>> it prevented people from misusing it.
>>>
>>> > At that time services map was a HashMap and could not fix the issue by
>>> > changing it to a ConcurrentHashMap since API method returns a HashMap.
>>> >
>>> > Currently anyone can get a copy of internal map (I think we can not use
>>> > clone() method since internal implementation is ConcurrentHashMap and
>>> > we
>>> > need to return a HashMap). And if they want to add or remove service
>>> > they have to use addService and removeService respectively.
>>> >
>>> > Keith, if you really need the internal map IMHO best way is to add a
>>> > new
>>> > API method.
>>>
>>> Ah, no. The "best way" is NOT TO OFFER ACCESS TO THE INTERNAL MAP.
>>>
>>> Keith's suggestion is to change the API so that it returns a Map - this
>>> would
>>> be much more correct since it increases the level of abstraction for the
>>> method, and would therefore let us, the implementors, freely decide how
>>> this
>>> should work internally. Right now we have problems because someone made
>>> this
>>> method overly specific, and this is what we should fix. (I was incorrect
>>> earlier when I said this wouldn't break people, btw - I was thinking
>>> about
>>> stuff like getServices().get("MyService"), but of course "HashMap foo =
>>> getServices()" would fail. I still think we should fix it.)
>>>
>>> My comment is that we should not only return a Map, we should change the
>>> implementation to make sure the Map is immutable, and make sure the
>>> JavaDoc
>>> explains what is going on.
>>
>> +1. Here the real problem is this API contains a hashMap instead of Map.
>>
>> What I got from the Keiths' first mail is that he need the API change to
>> return the internal map without copying. I suggested to add a new method if
>> he really need it so that only he use that method. I agree with you that
>> this is not a good change.
>>
>> thanks,
>> Amila.
>>>
>>>
>>> Make sense?
>>>
>>> --Glen
>>
>>
>>
>> --
>> Amila Suriarachchi
>> WSO2 Inc.
>> blog: http://amilachinthaka.blogspot.com/
>
>
>
> --
> Keith Chapman
> Senior Software Engineer
> WSO2 Inc.
> Oxygenating the Web Service Platform.
> http://wso2.org/
>
> blog: http://www.keith-chapman.org
>
