axis-java-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Davanum Srinivas <dava...@gmail.com>
Subject Re: Axis2 API design issue, Should we fix it?
Date Mon, 11 May 2009 13:00:24 GMT
+0 (Since opinions of certain people is what matters anyway!)

-- dims

On Mon, May 11, 2009 at 6:28 AM, Andreas Veithen
<andreas.veithen@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 <keithgchapman@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 <keithgchapman@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 <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
>>> >> > <amilasuriarachchi@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
>>
>



-- 
Davanum Srinivas :: http://davanum.wordpress.com

Mime
View raw message