You are viewing a plain text version of this content. The canonical link for it is here.
Posted to java-dev@axis.apache.org by keith chapman <ke...@gmail.com> on 2009/05/05 13:45:52 UTC
Axis2 API design issue, Should we fix it?
Hi all,
Due to a design issue in the Axis2 API we have implemented the getServices()
method as follows,
public HashMap<String, AxisService> getServices() {
HashMap<String, AxisService> hashMap = new HashMap<String,
AxisService>(this.allServices.size());
String key;
for (Iterator<String> iter = this.allServices.keySet().iterator();
iter.hasNext();){
key = iter.next();
hashMap.put(key, this.allServices.get(key));
}
return hashMap;
}
In my view this is highly inefficient, especially if you have plenty of
services in the system. Wouldn't it be better to fix the API and return a
Map instead of a HashMap? If we did that we could simple return allServices
instead of returning a copy of it.
If we are making this change I propose that we fix this for modules,
transports as well.
Thanks,
Keith.
--
Keith Chapman
Senior Software Engineer
WSO2 Inc.
Oxygenating the Web Service Platform.
http://wso2.org/
blog: http://www.keith-chapman.org
Re: Axis2 API design issue, Should we fix it?
Posted by keith chapman <ke...@gmail.com>.
+1 for making the return Map and returning
Collections.unmodifiableMap(allServices);
Thanks,
Keith.
On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <gl...@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.
>
> Make sense?
>
> --Glen
>
--
Keith Chapman
Senior Software Engineer
WSO2 Inc.
Oxygenating the Web Service Platform.
http://wso2.org/
blog: http://www.keith-chapman.org
Re: Axis2 API design issue, Should we fix it?
Posted by Amila Suriarachchi <am...@gmail.com>.
On Mon, May 11, 2009 at 1:45 PM, Andreas Veithen
<an...@gmail.com>wrote:
> +1, but we need to agree on the target release for this change.
I think this should not go to 1.5 and only apply to axis2 trunk (i.e 1.6).
thanks,
Amila.
>
>
> Andreas
>
> On Mon, May 11, 2009 at 09:57, keith chapman <ke...@gmail.com>
> wrote:
> > Shall we go ahead with this change then?
> >
> > Thanks,
> > Keith.
> >
> > On Wed, May 6, 2009 at 7:11 PM, Amila Suriarachchi
> > <am...@gmail.com> wrote:
> >>
> >>
> >> On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <gl...@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
> >
>
--
Amila Suriarachchi
WSO2 Inc.
blog: http://amilachinthaka.blogspot.com/
Re: Axis2 API design issue, Should we fix it?
Posted by keith chapman <ke...@gmail.com>.
+1 Sounds good to me.
Thanks,
Keith.
On Mon, May 11, 2009 at 3:58 PM, Andreas Veithen
<an...@gmail.com>wrote:
> I forgot about the refactoring of the clustering API, which is in
> trunk but didn't go into 1.5. That indeed means that that we can't do
> a 1.5.1 release from trunk.
>
> To summarize, the roadmap for the months after the 1.5 release (when
> is this actually going to happen?) would look like this:
>
> Release 1.5.1 (from 1.5 branch):
> - Critical bug fixes (if required)
> - Changes that didn't make it into 1.5 (e.g. the changes in
> axis2-saaj) and that can easily be merged from the trunk (if there is
> a user demand for this)
>
> Release 1.6 (current trunk):
> - API cleanup (concrete classes -> interfaces)
> - Refactoring of the clustering API (already in trunk)
> - Improvements to message builder/formatter API and implementations,
> as discussed in December
> - Clarifications and improvements in the transport API (if I'm not
> mistaken, there was a discussion around this some time ago)
> - ...
>
> If everybody is fine with this, then we should go ahead.
>
> Andreas
>
> On Mon, May 11, 2009 at 11:51, keith chapman <ke...@gmail.com>
> wrote:
> > AFAIK there have been quite a bit of development on the trunk since the
> > Axis2 1.5 branch was cut hence if we are to do a 1.5.1 release it will
> have
> > to be done off the 1.5 branch and not the trunk. Hence I don't see an
> issue
> > with doing this change on the trunk.
> >
> > Thanks,
> > Keith.
> >
> > On Mon, May 11, 2009 at 3:09 PM, Andreas Veithen <
> andreas.veithen@gmail.com>
> > wrote:
> >>
> >> 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 <ke...@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
> >> > <an...@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 <
> keithgchapman@gmail.com>
> >> >> wrote:
> >> >> > Shall we go ahead with this change then?
> >> >> >
> >> >> > Thanks,
> >> >> > Keith.
> >> >> >
> >> >> > On Wed, May 6, 2009 at 7:11 PM, Amila Suriarachchi
> >> >> > <am...@gmail.com> wrote:
> >> >> >>
> >> >> >>
> >> >> >> On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <
> glen@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
> >> >
> >
> >
> >
> > --
> > 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
Re: Axis2 API design issue, Should we fix it?
Posted by Davanum Srinivas <da...@gmail.com>.
+0 (Since opinions of certain people is what matters anyway!)
-- dims
On Mon, May 11, 2009 at 6:28 AM, Andreas Veithen
<an...@gmail.com> wrote:
> I forgot about the refactoring of the clustering API, which is in
> trunk but didn't go into 1.5. That indeed means that that we can't do
> a 1.5.1 release from trunk.
>
> To summarize, the roadmap for the months after the 1.5 release (when
> is this actually going to happen?) would look like this:
>
> Release 1.5.1 (from 1.5 branch):
> - Critical bug fixes (if required)
> - Changes that didn't make it into 1.5 (e.g. the changes in
> axis2-saaj) and that can easily be merged from the trunk (if there is
> a user demand for this)
>
> Release 1.6 (current trunk):
> - API cleanup (concrete classes -> interfaces)
> - Refactoring of the clustering API (already in trunk)
> - Improvements to message builder/formatter API and implementations,
> as discussed in December
> - Clarifications and improvements in the transport API (if I'm not
> mistaken, there was a discussion around this some time ago)
> - ...
>
> If everybody is fine with this, then we should go ahead.
>
> Andreas
>
> On Mon, May 11, 2009 at 11:51, keith chapman <ke...@gmail.com> wrote:
>> AFAIK there have been quite a bit of development on the trunk since the
>> Axis2 1.5 branch was cut hence if we are to do a 1.5.1 release it will have
>> to be done off the 1.5 branch and not the trunk. Hence I don't see an issue
>> with doing this change on the trunk.
>>
>> Thanks,
>> Keith.
>>
>> On Mon, May 11, 2009 at 3:09 PM, Andreas Veithen <an...@gmail.com>
>> wrote:
>>>
>>> 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 <ke...@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
>>> > <an...@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 <ke...@gmail.com>
>>> >> wrote:
>>> >> > Shall we go ahead with this change then?
>>> >> >
>>> >> > Thanks,
>>> >> > Keith.
>>> >> >
>>> >> > On Wed, May 6, 2009 at 7:11 PM, Amila Suriarachchi
>>> >> > <am...@gmail.com> wrote:
>>> >> >>
>>> >> >>
>>> >> >> On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <gl...@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
>>> >
>>
>>
>>
>> --
>> Keith Chapman
>> Senior Software Engineer
>> WSO2 Inc.
>> Oxygenating the Web Service Platform.
>> http://wso2.org/
>>
>> blog: http://www.keith-chapman.org
>>
>
--
Davanum Srinivas :: http://davanum.wordpress.com
Re: Axis2 API design issue, Should we fix it?
Posted by Andreas Veithen <an...@gmail.com>.
I forgot about the refactoring of the clustering API, which is in
trunk but didn't go into 1.5. That indeed means that that we can't do
a 1.5.1 release from trunk.
To summarize, the roadmap for the months after the 1.5 release (when
is this actually going to happen?) would look like this:
Release 1.5.1 (from 1.5 branch):
- Critical bug fixes (if required)
- Changes that didn't make it into 1.5 (e.g. the changes in
axis2-saaj) and that can easily be merged from the trunk (if there is
a user demand for this)
Release 1.6 (current trunk):
- API cleanup (concrete classes -> interfaces)
- Refactoring of the clustering API (already in trunk)
- Improvements to message builder/formatter API and implementations,
as discussed in December
- Clarifications and improvements in the transport API (if I'm not
mistaken, there was a discussion around this some time ago)
- ...
If everybody is fine with this, then we should go ahead.
Andreas
On Mon, May 11, 2009 at 11:51, keith chapman <ke...@gmail.com> wrote:
> AFAIK there have been quite a bit of development on the trunk since the
> Axis2 1.5 branch was cut hence if we are to do a 1.5.1 release it will have
> to be done off the 1.5 branch and not the trunk. Hence I don't see an issue
> with doing this change on the trunk.
>
> Thanks,
> Keith.
>
> On Mon, May 11, 2009 at 3:09 PM, Andreas Veithen <an...@gmail.com>
> wrote:
>>
>> 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 <ke...@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
>> > <an...@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 <ke...@gmail.com>
>> >> wrote:
>> >> > Shall we go ahead with this change then?
>> >> >
>> >> > Thanks,
>> >> > Keith.
>> >> >
>> >> > On Wed, May 6, 2009 at 7:11 PM, Amila Suriarachchi
>> >> > <am...@gmail.com> wrote:
>> >> >>
>> >> >>
>> >> >> On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <gl...@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
>> >
>
>
>
> --
> Keith Chapman
> Senior Software Engineer
> WSO2 Inc.
> Oxygenating the Web Service Platform.
> http://wso2.org/
>
> blog: http://www.keith-chapman.org
>
Re: Axis2 API design issue, Should we fix it?
Posted by keith chapman <ke...@gmail.com>.
AFAIK there have been quite a bit of development on the trunk since the
Axis2 1.5 branch was cut hence if we are to do a 1.5.1 release it will have
to be done off the 1.5 branch and not the trunk. Hence I don't see an issue
with doing this change on the trunk.
Thanks,
Keith.
On Mon, May 11, 2009 at 3:09 PM, Andreas Veithen
<an...@gmail.com>wrote:
> 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 <ke...@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.veithen@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 <ke...@gmail.com>
> >> wrote:
> >> > Shall we go ahead with this change then?
> >> >
> >> > Thanks,
> >> > Keith.
> >> >
> >> > On Wed, May 6, 2009 at 7:11 PM, Amila Suriarachchi
> >> > <am...@gmail.com> wrote:
> >> >>
> >> >>
> >> >> On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <gl...@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
> >
>
--
Keith Chapman
Senior Software Engineer
WSO2 Inc.
Oxygenating the Web Service Platform.
http://wso2.org/
blog: http://www.keith-chapman.org
Re: Axis2 API design issue, Should we fix it?
Posted by Andreas Veithen <an...@gmail.com>.
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 <ke...@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 <an...@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 <ke...@gmail.com>
>> wrote:
>> > Shall we go ahead with this change then?
>> >
>> > Thanks,
>> > Keith.
>> >
>> > On Wed, May 6, 2009 at 7:11 PM, Amila Suriarachchi
>> > <am...@gmail.com> wrote:
>> >>
>> >>
>> >> On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <gl...@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
>
Re: Axis2 API design issue, Should we fix it?
Posted by keith chapman <ke...@gmail.com>.
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
<an...@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 <ke...@gmail.com>
> wrote:
> > Shall we go ahead with this change then?
> >
> > Thanks,
> > Keith.
> >
> > On Wed, May 6, 2009 at 7:11 PM, Amila Suriarachchi
> > <am...@gmail.com> wrote:
> >>
> >>
> >> On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <gl...@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
Re: Axis2 API design issue, Should we fix it?
Posted by Andreas Veithen <an...@gmail.com>.
+1, but we need to agree on the target release for this change.
Andreas
On Mon, May 11, 2009 at 09:57, keith chapman <ke...@gmail.com> wrote:
> Shall we go ahead with this change then?
>
> Thanks,
> Keith.
>
> On Wed, May 6, 2009 at 7:11 PM, Amila Suriarachchi
> <am...@gmail.com> wrote:
>>
>>
>> On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <gl...@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
>
Re: Axis2 API design issue, Should we fix it?
Posted by keith chapman <ke...@gmail.com>.
Shall we go ahead with this change then?
Thanks,
Keith.
On Wed, May 6, 2009 at 7:11 PM, Amila Suriarachchi <
amilasuriarachchi@gmail.com> wrote:
>
>
> On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <gl...@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
Re: Axis2 API design issue, Should we fix it?
Posted by Amila Suriarachchi <am...@gmail.com>.
On Wed, May 6, 2009 at 4:33 PM, Glen Daniels <gl...@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/
Re: Axis2 API design issue, Should we fix it?
Posted by Glen Daniels <gl...@thoughtcraft.com>.
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.
Make sense?
--Glen
Re: Axis2 API design issue, Should we fix it?
Posted by Amila Suriarachchi <am...@gmail.com>.
On Wed, May 6, 2009 at 9:49 AM, Glen Daniels <gl...@thoughtcraft.com> wrote:
> keith chapman wrote:
> > Um - we aren't currently returning the actual Map, we're returning a
> > clone of
> > it (just done manually instead of using clone()). You had suggested
> > returning the actual Map itself, which is what I was reacting to
> > above. I'm
> > not saying the API should go away.
> >
> > The reason we have implemented a clone is the limitation in the API. If
> > the return type was Map we could have returned the allServices map.
> > Wouldn't this clone be expensive when there are lots of services
> deployed?
>
> 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. There is (and should be) a contract about
> how you add and remove services - if you don't follow the contract, then
> stuff like engaging global modules doesn't happen. In our case this stuff
> lives in the code in addServiceGroup() (whether or not that's where it
> should
> live is debatable, but the point is that it exists). If someone were to
> mess
> around with the actual contents of allServices, that would produce
> undefined
> results.
>
> This is basic OOP - don't expose the inner workings of your class by
> offering
> mutable references to private data.
>
> Instead of cloning, we could just use Collections.unmodifiableMap() for
> speed, but we'd need to be clear (in the code and the JavaDoc) whether
> we're
> returning a "moment in time" (i.e. a clone()) or a "live view" (which is I
> think what unmodifiableMap() gives you) - i.e. if some other thread deploys
> a
> service while I have the reference, can I see the new service or not?
As I remember this change was done to avoid concurrent modifications to
service map[1].
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.
thanks,
Amila.
[1]
http://svn.apache.org/viewvc/webservices/axis2/trunk/java/modules/kernel/src/org/apache/axis2/engine/AxisConfiguration.java?r1=679658&r2=684775
>
>
> --Glen
>
--
Amila Suriarachchi
WSO2 Inc.
blog: http://amilachinthaka.blogspot.com/
Re: Axis2 API design issue, Should we fix it?
Posted by Glen Daniels <gl...@thoughtcraft.com>.
keith chapman wrote:
> Um - we aren't currently returning the actual Map, we're returning a
> clone of
> it (just done manually instead of using clone()). You had suggested
> returning the actual Map itself, which is what I was reacting to
> above. I'm
> not saying the API should go away.
>
> The reason we have implemented a clone is the limitation in the API. If
> the return type was Map we could have returned the allServices map.
> Wouldn't this clone be expensive when there are lots of services deployed?
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. There is (and should be) a contract about
how you add and remove services - if you don't follow the contract, then
stuff like engaging global modules doesn't happen. In our case this stuff
lives in the code in addServiceGroup() (whether or not that's where it should
live is debatable, but the point is that it exists). If someone were to mess
around with the actual contents of allServices, that would produce undefined
results.
This is basic OOP - don't expose the inner workings of your class by offering
mutable references to private data.
Instead of cloning, we could just use Collections.unmodifiableMap() for
speed, but we'd need to be clear (in the code and the JavaDoc) whether we're
returning a "moment in time" (i.e. a clone()) or a "live view" (which is I
think what unmodifiableMap() gives you) - i.e. if some other thread deploys a
service while I have the reference, can I see the new service or not?
--Glen
Re: Axis2 API design issue, Should we fix it?
Posted by keith chapman <ke...@gmail.com>.
See comments inline.
On Wed, May 6, 2009 at 8:39 AM, Glen Daniels <gl...@thoughtcraft.com> wrote:
> keith chapman wrote:
> > > In my view this is highly inefficient, especially if you have
> > plenty of
> > > services in the system. Wouldn't it be better to fix the API and
> > return
> > > a Map instead of a HashMap? If we did that we could simple return
> > > allServices instead of returning a copy of it.
> >
> > You don't want to return an actual reference to a mutable object
> > that backs a
> > significant data model - otherwise people could just get that Map and
> > (mistakenly or maliciously) randomly add and delete services.
> >
> > Agreed. But now that we've been doing it people may have code that
> > expect it to be there. So we need a mechanism for returning this map.
>
> Um - we aren't currently returning the actual Map, we're returning a clone
> of
> it (just done manually instead of using clone()). You had suggested
> returning the actual Map itself, which is what I was reacting to above.
> I'm
> not saying the API should go away.
The reason we have implemented a clone is the limitation in the API. If the
return type was Map we could have returned the allServices map. Wouldn't
this clone be expensive when there are lots of services deployed?
+1 to adding JavaDoc saying that please use this map for reading purposes
only.
Thanks,
Keith.
>
>
> <rant>
> Of course, if we had any kind of reasonable JavaDoc, there would be a clear
> indication in the API docs that the returned HashMap was in fact a clone
> and
> that inserting or removing things from it would have no effect on the
> system.
> </rant>
>
> Thanks,
> --Glen
>
--
Keith Chapman
Senior Software Engineer
WSO2 Inc.
Oxygenating the Web Service Platform.
http://wso2.org/
blog: http://www.keith-chapman.org
Re: Axis2 API design issue, Should we fix it?
Posted by Glen Daniels <gl...@thoughtcraft.com>.
keith chapman wrote:
> > In my view this is highly inefficient, especially if you have
> plenty of
> > services in the system. Wouldn't it be better to fix the API and
> return
> > a Map instead of a HashMap? If we did that we could simple return
> > allServices instead of returning a copy of it.
>
> You don't want to return an actual reference to a mutable object
> that backs a
> significant data model - otherwise people could just get that Map and
> (mistakenly or maliciously) randomly add and delete services.
>
> Agreed. But now that we've been doing it people may have code that
> expect it to be there. So we need a mechanism for returning this map.
Um - we aren't currently returning the actual Map, we're returning a clone of
it (just done manually instead of using clone()). You had suggested
returning the actual Map itself, which is what I was reacting to above. I'm
not saying the API should go away.
<rant>
Of course, if we had any kind of reasonable JavaDoc, there would be a clear
indication in the API docs that the returned HashMap was in fact a clone and
that inserting or removing things from it would have no effect on the system.
</rant>
Thanks,
--Glen
Re: Axis2 API design issue, Should we fix it?
Posted by keith chapman <ke...@gmail.com>.
On Wed, May 6, 2009 at 1:50 AM, Glen Daniels <gl...@thoughtcraft.com> wrote:
> Hi Keith!
>
> keith chapman wrote:
> > Due to a design issue in the Axis2 API we have implemented the
> > getServices() method as follows,
> >
> > public HashMap<String, AxisService> getServices() {
> > HashMap<String, AxisService> hashMap = new HashMap<String,
> > AxisService>(this.allServices.size());
> > String key;
> > for (Iterator<String> iter =
> > this.allServices.keySet().iterator(); iter.hasNext();){
> > key = iter.next();
> > hashMap.put(key, this.allServices.get(key));
> > }
> > return hashMap;
> > }
> >
> > In my view this is highly inefficient, especially if you have plenty of
> > services in the system. Wouldn't it be better to fix the API and return
> > a Map instead of a HashMap? If we did that we could simple return
> > allServices instead of returning a copy of it.
>
> You don't want to return an actual reference to a mutable object that backs
> a
> significant data model - otherwise people could just get that Map and
> (mistakenly or maliciously) randomly add and delete services.
Agreed. But now that we've been doing it people may have code that expect it
to be there. So we need a mechanism for returning this map.
>
> However, your point about the HashMap in the API is entirely right - we
> should make it a Map (which shouldn't break anyone already using it in it's
> more specific form). Also, I'm not sure why we're not just returning
> allServices.clone()... NIH? :)
I was wondering the same. So your in agreement for changing the API and
returning a Map. Cool.
Thanks,
Keith.
>
>
> > If we are making this change I propose that we fix this for modules,
> > transports as well.
>
> Sure.
>
> --Glen
>
--
Keith Chapman
Senior Software Engineer
WSO2 Inc.
Oxygenating the Web Service Platform.
http://wso2.org/
blog: http://www.keith-chapman.org
Re: Axis2 API design issue, Should we fix it?
Posted by Glen Daniels <gl...@thoughtcraft.com>.
Hi Keith!
keith chapman wrote:
> Due to a design issue in the Axis2 API we have implemented the
> getServices() method as follows,
>
> public HashMap<String, AxisService> getServices() {
> HashMap<String, AxisService> hashMap = new HashMap<String,
> AxisService>(this.allServices.size());
> String key;
> for (Iterator<String> iter =
> this.allServices.keySet().iterator(); iter.hasNext();){
> key = iter.next();
> hashMap.put(key, this.allServices.get(key));
> }
> return hashMap;
> }
>
> In my view this is highly inefficient, especially if you have plenty of
> services in the system. Wouldn't it be better to fix the API and return
> a Map instead of a HashMap? If we did that we could simple return
> allServices instead of returning a copy of it.
You don't want to return an actual reference to a mutable object that backs a
significant data model - otherwise people could just get that Map and
(mistakenly or maliciously) randomly add and delete services.
However, your point about the HashMap in the API is entirely right - we
should make it a Map (which shouldn't break anyone already using it in it's
more specific form). Also, I'm not sure why we're not just returning
allServices.clone()... NIH? :)
> If we are making this change I propose that we fix this for modules,
> transports as well.
Sure.
--Glen