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 Wed, 08 Mar 2017 11:47:18 GMT
IGNITE-4564 is ready for review.
.

1. JdbcCheckpointSpi implements MBean interface, but never registered as
MBean. Should it be fixed somehow?
2. Should we move MBeans interface metods from SPI implementation to MBean
implementation or just make calls from MBean to SPI as it done for now,
some of these methods can be used in unittests?



On Mon, Feb 13, 2017 at 5:50 PM, Alexey Goncharuk <
alexey.goncharuk@gmail.com> wrote:

> 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
> > >
> >
>



-- 
Best regards,
Andrey V. Mashenkov

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