Ensure that builder approach is used for all setters in public API

[Andrew Mashenkov]

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 <
[hidden email]> wrote:

> Andrey, Yakov,
>
> An MBean for eviction policy is registered in GridCacheProcessor#prepare().
>
> --AG
>
> 2017-02-09 18:53 GMT+03:00 Yakov Zhdanov <[hidden email]>:
>
> > 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" <[hidden email]>
> > 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 <[hidden email]>
> > wrote:
> > >
> > > > +1 to Vladimir suggestion
> > > >
> > > > --Yakov
> > > >
> > > > 2017-02-07 20:50 GMT+07:00 Vladimir Ozerov <[hidden email]>:
> > > >
> > > > > 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 <
> > > > > [hidden email]
> > > > > > 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 <
> > > > > > [hidden email]> wrote:
> > > > > >
> > > > > > > Andrey,
> > > > > > >
> > > > > > > Can you list all setters that we have on MBeans?
> > > > > > >
> > > > > > > -Val
> > > > > > >
> > > > > > > On Mon, Feb 6, 2017 at 2:21 PM, Andrey Mashenkov <
> > > > > > > [hidden email]
> > > > > > > > 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 <
> > > > > [hidden email]
> > > > > > >
> > > > > > > > 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 <
> > > > > > > > > [hidden email]> 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 <
> > > > [hidden email]>
> > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Sorry, “public modifications” -> “public APIs”
> > > > > > > > > > >
> > > > > > > > > > > —
> > > > > > > > > > > Denis
> > > > > > > > > > >
> > > > > > > > > > > > On Feb 3, 2017, at 10:03 AM, Denis Magda <
> > > > [hidden email]>
> > > > > > > > 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 <
> > > > > > > > > > > [hidden email]> 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 <
> > > > > > > > > > [hidden email]>
> > > > > > > > > > > >> 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 <
> > > > > > > > > > > >>> [hidden email]> 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 <
> > > > > > > > > > > >>>> [hidden email]
> > > > > > > > > > > >>>>> 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 <
> > > > > > > > > > > >>>>> [hidden email]> 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 <
> > > > > > > > > > > >>>>>> [hidden email]
> > > > > > > > > > > >>>>>>> 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
> > > > > > > > > > > >>>>>>>
> > > > > > > > > > > >>>>>>
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>>> --
> > > > > > > > > > > >>>>> С уважением,
> > > > > > > > > > > >>>>> Машенков Андрей Владимирович
> > > > > > > > > > > >>>>> Тел. [phone]
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>>> Best regards,
> > > > > > > > > > > >>>>> Andrey V. Mashenkov
> > > > > > > > > > > >>>>> Cerr: [phone]
> > > > > > > > > > > >>>>>
> > > > > > > > > > > >>>>
> > > > > > > > > > > >>>
> > > > > > > > > > > >>
> > > > > > > > > > > >>
> > > > > > > > > > > >>
> > > > > > > > > > > >> --
> > > > > > > > > > > >> С уважением,
> > > > > > > > > > > >> Машенков Андрей Владимирович
> > > > > > > > > > > >> Тел. [phone]
> > > > > > > > > > > >>
> > > > > > > > > > > >> Best regards,
> > > > > > > > > > > >> Andrey V. Mashenkov
> > > > > > > > > > > >> Cerr: [phone]
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > С уважением,
> > > > > > > > Машенков Андрей Владимирович
> > > > > > > > Тел. [phone]
> > > > > > > >
> > > > > > > > Best regards,
> > > > > > > > Andrey V. Mashenkov
> > > > > > > > Cerr: [phone]
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Best regards,
> > > > > > Andrey V. Mashenkov
> > > > > >
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > Best regards,
> > > Andrey V. Mashenkov
> > >
> >
>

--
Best regards,
Andrey V. Mashenkov