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

classic Classic list List threaded Threaded
21 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Andrew 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
Reply | Threaded
Open this post in threaded view
|

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

Valentin Kulichenko
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.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
>
Reply | Threaded
Open this post in threaded view
|

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

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



--
С уважением,
Машенков Андрей Владимирович
Тел. +7-921-932-61-82

Best regards,
Andrey V. Mashenkov
Cerr: +7-921-932-61-82
Reply | Threaded
Open this post in threaded view
|

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

Valentin Kulichenko
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
> > >
> >
>
>
>
> --
> С уважением,
> Машенков Андрей Владимирович
> Тел. +7-921-932-61-82
>
> Best regards,
> Andrey V. Mashenkov
> Cerr: +7-921-932-61-82
>
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Ozerov
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
> > > >
> > >
> >
> >
> >
> > --
> > С уважением,
> > Машенков Андрей Владимирович
> > Тел. +7-921-932-61-82
> >
> > Best regards,
> > Andrey V. Mashenkov
> > Cerr: +7-921-932-61-82
> >
>
Reply | Threaded
Open this post in threaded view
|

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

Andrew Mashenkov
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
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > С уважением,
> > > Машенков Андрей Владимирович
> > > Тел. +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
Reply | Threaded
Open this post in threaded view
|

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

Andrew Mashenkov
I've found no restriction on return type in MBeans spicification [1]:

A *setter* is any public method that takes a single parameter and whose
name begins with set.


[1] https://docs.oracle.com/javase/tutorial/jmx/mbeans/standard.html


On Fri, Feb 3, 2017 at 11: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.IgfsPerBlockLruEvictio
>> nPolicyM
>> > > > 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.FifoQueueCollision
>> SpiMBean
>> > > > >
>> > > > > 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
Reply | Threaded
Open this post in threaded view
|

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

dmagda
In reply to this post by Andrew Mashenkov
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
>>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> С уважением,
>>>> Машенков Андрей Владимирович
>>>> Тел. +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

Reply | Threaded
Open this post in threaded view
|

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

dmagda
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
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> С уважением,
>>>>> Машенков Андрей Владимирович
>>>>> Тел. +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
>

Reply | Threaded
Open this post in threaded view
|

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 <[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
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> --
> >>>>> С уважением,
> >>>>> Машенков Андрей Владимирович
> >>>>> Тел. +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
> >
>
>
Reply | Threaded
Open this post in threaded view
|

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

Vladimir Ozerov
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
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> --
> > >>>>> С уважением,
> > >>>>> Машенков Андрей Владимирович
> > >>>>> Тел. +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
> > >
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

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

Andrew 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 <[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
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>> --
> > > >>>>> С уважением,
> > > >>>>> Машенков Андрей Владимирович
> > > >>>>> Тел. +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
Reply | Threaded
Open this post in threaded view
|

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 <[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
> > > > >>>>>>>
> > > > >>>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>> --
> > > > >>>>> С уважением,
> > > > >>>>> Машенков Андрей Владимирович
> > > > >>>>> Тел. +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
>
Reply | Threaded
Open this post in threaded view
|

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

Andrew Mashenkov
Val,

void setBatchSize(int batchSize)
void setMaxMemorySize(long maxMemSize)
void setMaxSize(int max)
void setExcludePaths(Collection<String> excludePaths)
void setMaxBlocks(int maxBlocks)
void setParallelJobsNumber(int num)
void setWaitingJobsNumber(int num)

https://ignite.apache.org/releases/1.8.0/javadoc/org/apache/ignite/cache/eviction/fifo/FifoEvictionPolicyMBean.html
https://ignite.apache.org/releases/1.8.0/javadoc/org/apache/ignite/cache/eviction/igfs/IgfsPerBlockLruEvictionPolicyMXBean.html
https://ignite.apache.org/releases/1.8.0/javadoc/org/apache/ignite/cache/eviction/lru/LruEvictionPolicyMBean.html
https://ignite.apache.org/releases/1.8.0/javadoc/org/apache/ignite/cache/eviction/sorted/SortedEvictionPolicyMBean.html
https://ignite.apache.org/releases/1.8.0/javadoc/org/apache/ignite/spi/collision/fifoqueue/FifoQueueCollisionSpiMBean.html

On Tue, Feb 7, 2017 at 2:18 AM, Valentin Kulichenko <
[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
> > > > > >>>>>>>
> > > > > >>>>>>
> > > > > >>>>>
> > > > > >>>>>
> > > > > >>>>>
> > > > > >>>>> --
> > > > > >>>>> С уважением,
> > > > > >>>>> Машенков Андрей Владимирович
> > > > > >>>>> Тел. +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
Reply | Threaded
Open this post in threaded view
|

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 <[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/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 <
> [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
> > > > > > >>>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>
> > > > > > >>>>>
> > > > > > >>>>>
> > > > > > >>>>> --
> > > > > > >>>>> С уважением,
> > > > > > >>>>> Машенков Андрей Владимирович
> > > > > > >>>>> Тел. +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
>
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]>
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 <
> [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
> > > > > > > >>>>>>>
> > > > > > > >>>>>>
> > > > > > > >>>>>
> > > > > > > >>>>>
> > > > > > > >>>>>
> > > > > > > >>>>> --
> > > > > > > >>>>> С уважением,
> > > > > > > >>>>> Машенков Андрей Владимирович
> > > > > > > >>>>> Тел. +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
> >
>
Reply | Threaded
Open this post in threaded view
|

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

yzhdanov
In reply to this post by Vladimir Ozerov
+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
> > > > > > > >>>>>>>
> > > > > > > >>>>>>
> > > > > > > >>>>>
> > > > > > > >>>>>
> > > > > > > >>>>>
> > > > > > > >>>>> --
> > > > > > > >>>>> С уважением,
> > > > > > > >>>>> Машенков Андрей Владимирович
> > > > > > > >>>>> Тел. +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
> >
>
Reply | Threaded
Open this post in threaded view
|

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

Andrew 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 <[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
> > > > > > > > >>>>>>>
> > > > > > > > >>>>>>
> > > > > > > > >>>>>
> > > > > > > > >>>>>
> > > > > > > > >>>>>
> > > > > > > > >>>>> --
> > > > > > > > >>>>> С уважением,
> > > > > > > > >>>>> Машенков Андрей Владимирович
> > > > > > > > >>>>> Тел. +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
Reply | Threaded
Open this post in threaded view
|

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

yzhdanov
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
> > > > > > > > > >>>>>>>
> > > > > > > > > >>>>>>
> > > > > > > > > >>>>>
> > > > > > > > > >>>>>
> > > > > > > > > >>>>>
> > > > > > > > > >>>>> --
> > > > > > > > > >>>>> С уважением,
> > > > > > > > > >>>>> Машенков Андрей Владимирович
> > > > > > > > > >>>>> Тел. +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
>
Reply | Threaded
Open this post in threaded view
|

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 <[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
> > > > > > > > > > >>>>>>>
> > > > > > > > > > >>>>>>
> > > > > > > > > > >>>>>
> > > > > > > > > > >>>>>
> > > > > > > > > > >>>>>
> > > > > > > > > > >>>>> --
> > > > > > > > > > >>>>> С уважением,
> > > > > > > > > > >>>>> Машенков Андрей Владимирович
> > > > > > > > > > >>>>> Тел. +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
> >
>
12