ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexey Goncharuk <alexey.goncha...@gmail.com>
Subject Re: Ensure that builder approach is used for all setters in public API
Date Mon, 13 Feb 2017 14:50:31 GMT
Andrey, Yakov,

An MBean for eviction policy is registered in GridCacheProcessor#prepare().

--AG

2017-02-09 18:53 GMT+03:00 Yakov Zhdanov <yzhdanov@apache.org>:

> Wow! This is the regression (from long ago version) if true.
>
> As far as having mbean to manage eviction policy on the fly - why not? This
> is handy.
>
> --
> Yakov Zhdanov
>
> On Feb 9, 2017 9:09 PM, "Andrey Mashenkov" <andrey.mashenkov@gmail.com>
> wrote:
>
> > Folks,
> >
> > I've found no mention in ignite code where EvictionPolicy used as MBean
> and
> > it seems it is never registered as MBean.
> > Is it really need to have MBean interfaces for EvictionPolicy
> > implementations?
> >
> >
> >
> > On Wed, Feb 8, 2017 at 7:23 AM, Yakov Zhdanov <yzhdanov@apache.org>
> wrote:
> >
> > > +1 to Vladimir suggestion
> > >
> > > --Yakov
> > >
> > > 2017-02-07 20:50 GMT+07:00 Vladimir Ozerov <vozerov@gridgain.com>:
> > >
> > > > 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/IgfsPerBlockLruEvictionPolicyM
> > > > XBean.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
> > > > >
> > > >
> > >
> >
> >
> >
> > --
> > Best regards,
> > Andrey V. Mashenkov
> >
>

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