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

[Andrey 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 <
alexey.goncha...@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 :
>
> > 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" 
> > 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 
> > wrote:
> > >
> > > > +1 to Vladimir suggestion
> > > >
> > > > --Yakov
> > > >
> > > > 2017-02-07 20:50 GMT+07:00 Vladimir Ozerov :
> > > >
> > > > > 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.mashen...@gmail.com
> > > > > > wrote:
> > > > >
> > > > > > Val,
> > > > > >
> > > > > > void setBatchSize(int batchSize)
> > > > > > void setMaxMemorySize(long maxMemSize)
> > > > > > void setMaxSize(int max)
> > > > > > void setExcludePaths(Collection 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.kuliche...@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.mashen...@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 <
> > > > > voze...@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.kuliche...@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
> > 

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

[Alexey Goncharuk]

Andrey, Yakov,

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

--AG

2017-02-09 18:53 GMT+03:00 Yakov Zhdanov :

> 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" 
> 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 
> wrote:
> >
> > > +1 to Vladimir suggestion
> > >
> > > --Yakov
> > >
> > > 2017-02-07 20:50 GMT+07:00 Vladimir Ozerov :
> > >
> > > > 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.mashen...@gmail.com
> > > > > wrote:
> > > >
> > > > > Val,
> > > > >
> > > > > void setBatchSize(int batchSize)
> > > > > void setMaxMemorySize(long maxMemSize)
> > > > > void setMaxSize(int max)
> > > > > void setExcludePaths(Collection 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.kuliche...@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.mashen...@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 <
> > > > voze...@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.kuliche...@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
> > > > > > > 

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

[Yakov Zhdanov]

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" 
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  wrote:
>
> > +1 to Vladimir suggestion
> >
> > --Yakov
> >
> > 2017-02-07 20:50 GMT+07:00 Vladimir Ozerov :
> >
> > > 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.mashen...@gmail.com
> > > > wrote:
> > >
> > > > Val,
> > > >
> > > > void setBatchSize(int batchSize)
> > > > void setMaxMemorySize(long maxMemSize)
> > > > void setMaxSize(int max)
> > > > void setExcludePaths(Collection 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.kuliche...@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.mashen...@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 <
> > > voze...@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.kuliche...@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 <
> > dma...@apache.org>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > > Sorry, “public modifications” -> “public APIs”
> > > > > > > > >
> > > > > > > > > —
> > > > > > > > > Denis
> > > > > > > > >
> > > > > > > > > > On Feb 3, 2017, at 10:03 AM, Denis Magda <
> > dma...@apache.org>
> > > > > > wrote:
> > > > > > > > > >
> > 

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

[Andrey Mashenkov]

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

> +1 to Vladimir suggestion
>
> --Yakov
>
> 2017-02-07 20:50 GMT+07:00 Vladimir Ozerov :
>
> > 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.mashen...@gmail.com
> > > wrote:
> >
> > > Val,
> > >
> > > void setBatchSize(int batchSize)
> > > void setMaxMemorySize(long maxMemSize)
> > > void setMaxSize(int max)
> > > void setExcludePaths(Collection 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.kuliche...@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.mashen...@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 <
> > voze...@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.kuliche...@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 <
> dma...@apache.org>
> > > > > wrote:
> > > > > > >
> > > > > > > > Sorry, “public modifications” -> “public APIs”
> > > > > > > >
> > > > > > > > —
> > > > > > > > Denis
> > > > > > > >
> > > > > > > > > On Feb 3, 2017, at 10:03 AM, Denis Magda <
> dma...@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  > > > > > > > confluence/display/IGNITE/Apache+Ignite+2.0+Migration+Guide>
> > > > > > > > >
> > > > > > > > > —
> > > > > > > > > Denis
> > > > > > > > >
> > > > > > > > >> On Feb 3, 2017, at 12:24 AM, Andrey 

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

[Yakov Zhdanov]

+1 to Vladimir suggestion

--Yakov

2017-02-07 20:50 GMT+07:00 Vladimir Ozerov :

> 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.mashen...@gmail.com
> > wrote:
>
> > Val,
> >
> > void setBatchSize(int batchSize)
> > void setMaxMemorySize(long maxMemSize)
> > void setMaxSize(int max)
> > void setExcludePaths(Collection 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.kuliche...@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.mashen...@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 <
> voze...@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.kuliche...@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 
> > > > wrote:
> > > > > >
> > > > > > > Sorry, “public modifications” -> “public APIs”
> > > > > > >
> > > > > > > —
> > > > > > > Denis
> > > > > > >
> > > > > > > > On Feb 3, 2017, at 10:03 AM, Denis Magda 
> > > > 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  > > > > > > confluence/display/IGNITE/Apache+Ignite+2.0+Migration+Guide>
> > > > > > > >
> > > > > > > > —
> > > > > > > > Denis
> > > > > > > >
> > > > > > > >> On Feb 3, 2017, at 12:24 AM, Andrey Mashenkov <
> > > > > > > andrey.mashen...@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 <
> > > > > > 

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

[Valentin Kulichenko]

Vova,

You mean that MBean implementation will encapsulate the corresponding SPI
and delegate to its getters and setters as needed? If so, I like it, sounds
like a very clean and flexible approach.

-Val

On Tue, Feb 7, 2017 at 5:50 AM, Vladimir Ozerov 
wrote:

> 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.mashen...@gmail.com
> > wrote:
>
> > Val,
> >
> > void setBatchSize(int batchSize)
> > void setMaxMemorySize(long maxMemSize)
> > void setMaxSize(int max)
> > void setExcludePaths(Collection 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.kuliche...@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.mashen...@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 <
> voze...@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.kuliche...@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 
> > > > wrote:
> > > > > >
> > > > > > > Sorry, “public modifications” -> “public APIs”
> > > > > > >
> > > > > > > —
> > > > > > > Denis
> > > > > > >
> > > > > > > > On Feb 3, 2017, at 10:03 AM, Denis Magda 
> > > > 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  > > > > > > confluence/display/IGNITE/Apache+Ignite+2.0+Migration+Guide>
> > > > > > > >
> > > > > > > > —
> > > > > > > > Denis
> > > > > > > >
> > > > > > > >> On Feb 3, 2017, at 12:24 AM, Andrey Mashenkov <
> > > > > > > andrey.mashen...@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 

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

[Vladimir Ozerov]

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

> Val,
>
> void setBatchSize(int batchSize)
> void setMaxMemorySize(long maxMemSize)
> void setMaxSize(int max)
> void setExcludePaths(Collection 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.kuliche...@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.mashen...@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  >
> > > 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.kuliche...@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 
> > > wrote:
> > > > >
> > > > > > Sorry, “public modifications” -> “public APIs”
> > > > > >
> > > > > > —
> > > > > > Denis
> > > > > >
> > > > > > > On Feb 3, 2017, at 10:03 AM, Denis Magda 
> > > 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  > > > > > confluence/display/IGNITE/Apache+Ignite+2.0+Migration+Guide>
> > > > > > >
> > > > > > > —
> > > > > > > Denis
> > > > > > >
> > > > > > >> On Feb 3, 2017, at 12:24 AM, Andrey Mashenkov <
> > > > > > andrey.mashen...@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 <
> > > > > voze...@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

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

[Andrey Mashenkov]

Val,

void setBatchSize(int batchSize)
void setMaxMemorySize(long maxMemSize)
void setMaxSize(int max)
void setExcludePaths(Collection 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.kuliche...@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.mashen...@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 
> > 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.kuliche...@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 
> > wrote:
> > > >
> > > > > Sorry, “public modifications” -> “public APIs”
> > > > >
> > > > > —
> > > > > Denis
> > > > >
> > > > > > On Feb 3, 2017, at 10:03 AM, Denis Magda 
> > 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  > > > > confluence/display/IGNITE/Apache+Ignite+2.0+Migration+Guide>
> > > > > >
> > > > > > —
> > > > > > Denis
> > > > > >
> > > > > >> On Feb 3, 2017, at 12:24 AM, Andrey Mashenkov <
> > > > > andrey.mashen...@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 <
> > > > voze...@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.kuliche...@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.mashen...@gmail.com
> > > > > > wrote:
> > > > > 
> > > > > > Val,
> > > > > >
> > > > > > Yes, you are right. I don't think we should care about
> > > compilation

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

[Valentin Kulichenko]

Andrey,

Can you list all setters that we have on MBeans?

-Val

On Mon, Feb 6, 2017 at 2:21 PM, Andrey Mashenkov  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 
> 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.kuliche...@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 
> wrote:
> > >
> > > > Sorry, “public modifications” -> “public APIs”
> > > >
> > > > —
> > > > Denis
> > > >
> > > > > On Feb 3, 2017, at 10:03 AM, Denis Magda 
> 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  > > > confluence/display/IGNITE/Apache+Ignite+2.0+Migration+Guide>
> > > > >
> > > > > —
> > > > > Denis
> > > > >
> > > > >> On Feb 3, 2017, at 12:24 AM, Andrey Mashenkov <
> > > > andrey.mashen...@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 <
> > > voze...@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.kuliche...@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.mashen...@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.kuliche...@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.mashen...@gmail.com
> > > > >>> wrote:
> > > > >>
> > > > >>> Hi Igniters,
> > > > >>>
> > > > >>>
> > > > >>> I'm working on IGNITE-4564 [1] to make our 

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

[Andrey Mashenkov]

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 
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.kuliche...@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  wrote:
> >
> > > Sorry, “public modifications” -> “public APIs”
> > >
> > > —
> > > Denis
> > >
> > > > On Feb 3, 2017, at 10:03 AM, Denis Magda  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  > > confluence/display/IGNITE/Apache+Ignite+2.0+Migration+Guide>
> > > >
> > > > —
> > > > Denis
> > > >
> > > >> On Feb 3, 2017, at 12:24 AM, Andrey Mashenkov <
> > > andrey.mashen...@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 <
> > voze...@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.kuliche...@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.mashen...@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.kuliche...@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.mashen...@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 

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

[Valentin Kulichenko]

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

> Sorry, “public modifications” -> “public APIs”
>
> —
> Denis
>
> > On Feb 3, 2017, at 10:03 AM, Denis Magda  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  confluence/display/IGNITE/Apache+Ignite+2.0+Migration+Guide>
> >
> > —
> > Denis
> >
> >> On Feb 3, 2017, at 12:24 AM, Andrey Mashenkov <
> andrey.mashen...@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 
> >> 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.kuliche...@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.mashen...@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.kuliche...@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.mashen...@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

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

[Denis Magda]

Sorry, “public modifications” -> “public APIs”

—
Denis

> On Feb 3, 2017, at 10:03 AM, Denis Magda  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
>  
> 
> 
> —
> Denis
> 
>> On Feb 3, 2017, at 12:24 AM, Andrey Mashenkov  
>> 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 
>> 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.kuliche...@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.mashen...@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.kuliche...@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.mashen...@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
> 

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

[Denis Magda]

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
 


—
Denis

> On Feb 3, 2017, at 12:24 AM, Andrey Mashenkov  
> 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 
> 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.kuliche...@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.mashen...@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.kuliche...@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.mashen...@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

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

[Andrey Mashenkov]

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

[jira] [Created] (IGNITE-4564) Ensure that builder approach is used for all setters in public API

[Vladimir Ozerov (JIRA)]

Vladimir Ozerov created IGNITE-4564:
---

 Summary: Ensure that builder approach is used for all setters in 
public API
 Key: IGNITE-4564
 URL: https://issues.apache.org/jira/browse/IGNITE-4564
 Project: Ignite
  Issue Type: Task
  Components: general
Affects Versions: 1.8
Reporter: Vladimir Ozerov
 Fix For: 2.0


*Problem*
We employed "builder" approach for some configuration classes:
{code}
class Configuration {
Configuration setSomething(Something);
}
{code}

This is very convenient for users. However, only part of our configs employ 
this approach.

*Task*
Let's make sure that all other parts of our API follow this rule.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)