Please note that changing the return types from concrete
implementations to interfaces will break binary compatibility. This
means that if we do the change on trunk now, the next release from
trunk should be a major release, i.e. 1.6. How are we going to handle
the case where we need to produce a new release quickly after 1.5 to
fix serious issues (don't forget that 1.4.1 was produces by merging
only selected changes from trunk to 1.4 and that therefore 1.5
contains probably lots of changes)? If we don't do the API changes
immediately, then we keep the option of doing a 1.5.1 maintenance
release from the trunk.
Andreas
On Mon, May 11, 2009 at 10:27, keith chapman <keithgchap...@gmail.com> wrote:
> I don't think we can put it into 1.5. Lets make the change in the trunk so
> that it will be available in the next release.
>
> Thanks,
> Keith.
>
> On Mon, May 11, 2009 at 1:45 PM, Andreas Veithen <andreas.veit...@gmail.com>
> wrote:
>>
>> +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
>> >
>
>
>
> --
> Keith Chapman
> Senior Software Engineer
> WSO2 Inc.
> Oxygenating the Web Service Platform.
> http://wso2.org/
>
> blog: http://www.keith-chapman.org
>
