ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrey Mashenkov <andrey.mashen...@gmail.com>
Subject Re: Ensure that builder approach is used for all setters in public API
Date Mon, 06 Feb 2017 22:21:50 GMT
Folks,

Changing MBeans setters signature is bad idea. AOP tests failed on TC with
this change.

On Mon, Feb 6, 2017 at 11:21 AM, Vladimir Ozerov <vozerov@gridgain.com>
wrote:

> Val,
>
> Good catch! Can we try leaving SPIs and base methods untouched? Will it API
> be consistent in this case?
>
> On Fri, Feb 3, 2017 at 10:22 PM, Valentin Kulichenko <
> valentin.kulichenko@gmail.com> wrote:
>
> > Folks,
> >
> > I tend to think that the problem is that we try to apply 'builder
> approach'
> > to *ALL* setters. Let's approach this smarter.
> >
> > This approach is actually applicable only for configuration setters
> > available on public API, i.e. it's only about setters on ***Configuration
> > classes and SPI *implementations*. For SPI interface methods like
> > 'CollisionSpi.setExternalCollisionListener' this makes no sense, I would
> > not touch them.
> >
> > The only thing I still don't like is MBeans. Returning something except
> > void on MBean interfaces look ugly, but without doing this we will break
> > API consistency on the implementation. Any ideas on how to approach this?
> >
> > -Val
> >
> > On Fri, Feb 3, 2017 at 10:29 AM, Denis Magda <dmagda@apache.org> wrote:
> >
> > > Sorry, “public modifications” -> “public APIs”
> > >
> > > —
> > > Denis
> > >
> > > > On Feb 3, 2017, at 10:03 AM, Denis Magda <dmagda@apache.org> wrote:
> > > >
> > > > Andrey,
> > > >
> > > > If the changes affect public modifications don’t forget to update
> this
> > > page:
> > > > https://cwiki.apache.org/confluence/display/IGNITE/
> > > Apache+Ignite+2.0+Migration+Guide <https://cwiki.apache.org/
> > > confluence/display/IGNITE/Apache+Ignite+2.0+Migration+Guide>
> > > >
> > > > —
> > > > Denis
> > > >
> > > >> On Feb 3, 2017, at 12:24 AM, Andrey Mashenkov <
> > > andrey.mashenkov@gmail.com> wrote:
> > > >>
> > > >> Vladimir,
> > > >>
> > > >> Ok. I'll go ahead with changing SPI interfaces and run TC test.
> > > >> I think, it would be better to have this branch merged to master as
> 2
> > > >> separate commits at least.
> > > >> And may be we should make changes of SPI interfaces in separate Jira
> > > >> ticket?
> > > >>
> > > >> On Fri, Feb 3, 2017 at 11:08 AM, Vladimir Ozerov <
> > vozerov@gridgain.com>
> > > >> wrote:
> > > >>
> > > >>> Andrey,
> > > >>>
> > > >>> This is very important change from usability standpoint. But my
> main
> > > >>> concern is changes to SPI *interfaces*. If we do so users who
> > > implemented
> > > >>> custom SPIs will have broken compatibility. On the other hand,
I
> > doubt
> > > >>> there will be too much affected users, and we break compilation
in
> AI
> > > 2.0
> > > >>> anyway. So looks like we can go ahead with it.
> > > >>>
> > > >>> Thoughts?
> > > >>>
> > > >>> On Fri, Feb 3, 2017 at 7:46 AM, Valentin Kulichenko <
> > > >>> valentin.kulichenko@gmail.com> wrote:
> > > >>>
> > > >>>> My only concern is MBean interfaces. These are not called
from
> code,
> > > but
> > > >>>> from MBean viewers, and I'm not sure setters not returning
voids
> > will
> > > be
> > > >>>> properly treated as setters by these viewers. This needs to
be
> > > checked.
> > > >>>>
> > > >>>> -Val
> > > >>>>
> > > >>>> On Thu, Feb 2, 2017 at 8:32 PM, Andrey Mashenkov <
> > > >>>> andrey.mashenkov@gmail.com
> > > >>>>> wrote:
> > > >>>>
> > > >>>>> Val,
> > > >>>>>
> > > >>>>> Yes, you are right. I don't think we should care about
> compilation
> > > >>>>> error on user side, as we break compatibility with previous
> > versions.
> > > >>>>> But we talk about public interfaces and may be someone
has some
> > cons
> > > >>>>> or suggestions?
> > > >>>>>
> > > >>>>> On Fri, Feb 3, 2017 at 5:31 AM, Valentin Kulichenko <
> > > >>>>> valentin.kulichenko@gmail.com> wrote:
> > > >>>>>
> > > >>>>>> Andrey,
> > > >>>>>>
> > > >>>>>> In which case compatibility is broken? If there is
a method that
> > > >>>> returns
> > > >>>>>> void and you change it to return some type, it doesn't
break
> > > >>> anything,
> > > >>>>>> because currently nobody can assign the result of
this method
> to a
> > > >>>>>> variable. I.e. in old code the returned value will
be always
> > > ignored,
> > > >>>>>> therefore it can be of any type.
> > > >>>>>>
> > > >>>>>> Is there anything else that I'm missing?
> > > >>>>>>
> > > >>>>>> -Val
> > > >>>>>>
> > > >>>>>> On Thu, Feb 2, 2017 at 3:49 AM, Andrey Mashenkov <
> > > >>>>>> andrey.mashenkov@gmail.com
> > > >>>>>>> wrote:
> > > >>>>>>
> > > >>>>>>> Hi Igniters,
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>> I'm working on IGNITE-4564 [1] to make our configuration
> classes
> > > >>> and
> > > >>>>> SPI
> > > >>>>>>> classes more convenient.
> > > >>>>>>>
> > > >>>>>>> There is no problem to change return type in setter
method
> > > >>> signatures
> > > >>>>>>> and override methods in child child classes to
make them return
> > > >>> more
> > > >>>>>>> accurate type.
> > > >>>>>>>
> > > >>>>>>> But, I found that we have set methods in some
of our interfaces
> > and
> > > >>>>>>> changing its signature may broke compatibility
with user
> > > >>>>> implementations.
> > > >>>>>>>
> > > >>>>>>> Here are example interfaces with setters:
> > > >>>>>>> org.apache.ignite.cache.eviction.fifo.FifoEvictionPolicyMBean
> > > >>>>>>> org.apache.ignite.cache.eviction.igfs.
> > > >>> IgfsPerBlockLruEvictionPolicyM
> > > >>>>>> XBean
> > > >>>>>>> org.apache.ignite.cache.eviction.lru.LruEvictionPolicyMBean
> > > >>>>>>> org.apache.ignite.cache.eviction.sorted.
> > SortedEvictionPolicyMBean
> > > >>>>>>> org.apache.ignite.spi.checkpoint.CheckpointSpi
> > > >>>>>>> org.apache.ignite.spi.collision.CollisionSpi
> > > >>>>>>> org.apache.ignite.spi.collision.fifoqueue.
> > > >>> FifoQueueCollisionSpiMBean
> > > >>>>>>>
> > > >>>>>>> However we have interfaces with NO setters
> > > >>>>>>> org.apache.ignite.spi.loadbalancing.adaptive.
> > > >>>>>>> AdaptiveLoadBalancingSpiMBean.
> > > >>>>>>>
> > > >>>>>>> What can we do with it?
> > > >>>>>>> Change signature of setters without regarding
compatibility? Or
> > may
> > > >>>> be
> > > >>>>> it
> > > >>>>>>> is possible to remove setters from some interfaces?
> > > >>>>>>>
> > > >>>>>>> Thought?
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>> [1] https://issues.apache.org/jira/browse/IGNITE-4564
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>> --
> > > >>>>> С уважением,
> > > >>>>> Машенков Андрей Владимирович
> > > >>>>> Тел. +7-921-932-61-82
> > > >>>>>
> > > >>>>> Best regards,
> > > >>>>> Andrey V. Mashenkov
> > > >>>>> Cerr: +7-921-932-61-82
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>
> > > >>
> > > >>
> > > >> --
> > > >> С уважением,
> > > >> Машенков Андрей Владимирович
> > > >> Тел. +7-921-932-61-82
> > > >>
> > > >> Best regards,
> > > >> Andrey V. Mashenkov
> > > >> Cerr: +7-921-932-61-82
> > > >
> > >
> > >
> >
>



-- 
С уважением,
Машенков Андрей Владимирович
Тел. +7-921-932-61-82

Best regards,
Andrey V. Mashenkov
Cerr: +7-921-932-61-82

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message