ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vladimir Ozerov <voze...@gridgain.com>
Subject Re: Ensure that builder approach is used for all setters in public API
Date Tue, 07 Feb 2017 13:50:12 GMT
Andrey, Valya,

There is another problem here. What is we decide to add some existing
setter method to MBean? If it has signature "T setSomething(...)", we will
not be able to do so. We need to understand how to deal with it, so that
possible further improvements to MBean-s are not compromised. Any ideas?
May be we should fully decouple MBeans into separate classes?

E.g. instead of:
FifoEvictionPolicy implements FifoEvictionPolicyMBean

we will have
FifoEvictionPolicy
FifoEvictionPolicyMBeanImpl implements FifoEvictionPolicyMBean

This way public API will be fully decoupled form JMX what seems reasonable
to me. Thoughts?

On Tue, Feb 7, 2017 at 4:31 PM, Andrey Mashenkov <andrey.mashenkov@gmail.com
> wrote:

> Val,
>
> void setBatchSize(int batchSize)
> void setMaxMemorySize(long maxMemSize)
> void setMaxSize(int max)
> void setExcludePaths(Collection<String> excludePaths)
> void setMaxBlocks(int maxBlocks)
> void setParallelJobsNumber(int num)
> void setWaitingJobsNumber(int num)
>
> https://ignite.apache.org/releases/1.8.0/javadoc/org/
> apache/ignite/cache/eviction/fifo/FifoEvictionPolicyMBean.html
> https://ignite.apache.org/releases/1.8.0/javadoc/org/
> apache/ignite/cache/eviction/igfs/IgfsPerBlockLruEvictionPolicyMXBean.html
> https://ignite.apache.org/releases/1.8.0/javadoc/org/
> apache/ignite/cache/eviction/lru/LruEvictionPolicyMBean.html
> https://ignite.apache.org/releases/1.8.0/javadoc/org/
> apache/ignite/cache/eviction/sorted/SortedEvictionPolicyMBean.html
> https://ignite.apache.org/releases/1.8.0/javadoc/org/
> apache/ignite/spi/collision/fifoqueue/FifoQueueCollisionSpiMBean.html
>
> On Tue, Feb 7, 2017 at 2:18 AM, Valentin Kulichenko <
> valentin.kulichenko@gmail.com> wrote:
>
> > Andrey,
> >
> > Can you list all setters that we have on MBeans?
> >
> > -Val
> >
> > On Mon, Feb 6, 2017 at 2:21 PM, Andrey Mashenkov <
> > andrey.mashenkov@gmail.com
> > > wrote:
> >
> > > 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
> > >
> >
>
>
>
> --
> Best regards,
> Andrey V. Mashenkov
>

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