Folks, Changing MBeans setters signature is bad idea. AOP tests failed on TC with this change.
On Mon, Feb 6, 2017 at 11:21 AM, Vladimir Ozerov <voze...@gridgain.com> wrote: > Val, > > Good catch! Can we try leaving SPIs and base methods untouched? Will it API > be consistent in this case? > > On Fri, Feb 3, 2017 at 10:22 PM, Valentin Kulichenko < > valentin.kuliche...@gmail.com> wrote: > > > Folks, > > > > I tend to think that the problem is that we try to apply 'builder > approach' > > to *ALL* setters. Let's approach this smarter. > > > > This approach is actually applicable only for configuration setters > > available on public API, i.e. it's only about setters on ***Configuration > > classes and SPI *implementations*. For SPI interface methods like > > 'CollisionSpi.setExternalCollisionListener' this makes no sense, I would > > not touch them. > > > > The only thing I still don't like is MBeans. Returning something except > > void on MBean interfaces look ugly, but without doing this we will break > > API consistency on the implementation. Any ideas on how to approach this? > > > > -Val > > > > On Fri, Feb 3, 2017 at 10:29 AM, Denis Magda <dma...@apache.org> wrote: > > > > > Sorry, “public modifications” -> “public APIs” > > > > > > — > > > Denis > > > > > > > On Feb 3, 2017, at 10:03 AM, Denis Magda <dma...@apache.org> wrote: > > > > > > > > Andrey, > > > > > > > > If the changes affect public modifications don’t forget to update > this > > > page: > > > > https://cwiki.apache.org/confluence/display/IGNITE/ > > > Apache+Ignite+2.0+Migration+Guide <https://cwiki.apache.org/ > > > confluence/display/IGNITE/Apache+Ignite+2.0+Migration+Guide> > > > > > > > > — > > > > Denis > > > > > > > >> On Feb 3, 2017, at 12:24 AM, Andrey Mashenkov < > > > andrey.mashen...@gmail.com> wrote: > > > >> > > > >> Vladimir, > > > >> > > > >> Ok. I'll go ahead with changing SPI interfaces and run TC test. > > > >> I think, it would be better to have this branch merged to master as > 2 > > > >> separate commits at least. > > > >> And may be we should make changes of SPI interfaces in separate Jira > > > >> ticket? > > > >> > > > >> On Fri, Feb 3, 2017 at 11:08 AM, Vladimir Ozerov < > > voze...@gridgain.com> > > > >> wrote: > > > >> > > > >>> Andrey, > > > >>> > > > >>> This is very important change from usability standpoint. But my > main > > > >>> concern is changes to SPI *interfaces*. If we do so users who > > > implemented > > > >>> custom SPIs will have broken compatibility. On the other hand, I > > doubt > > > >>> there will be too much affected users, and we break compilation in > AI > > > 2.0 > > > >>> anyway. So looks like we can go ahead with it. > > > >>> > > > >>> Thoughts? > > > >>> > > > >>> On Fri, Feb 3, 2017 at 7:46 AM, Valentin Kulichenko < > > > >>> valentin.kuliche...@gmail.com> wrote: > > > >>> > > > >>>> My only concern is MBean interfaces. These are not called from > code, > > > but > > > >>>> from MBean viewers, and I'm not sure setters not returning voids > > will > > > be > > > >>>> properly treated as setters by these viewers. This needs to be > > > checked. > > > >>>> > > > >>>> -Val > > > >>>> > > > >>>> On Thu, Feb 2, 2017 at 8:32 PM, Andrey Mashenkov < > > > >>>> andrey.mashen...@gmail.com > > > >>>>> wrote: > > > >>>> > > > >>>>> Val, > > > >>>>> > > > >>>>> Yes, you are right. I don't think we should care about > compilation > > > >>>>> error on user side, as we break compatibility with previous > > versions. > > > >>>>> But we talk about public interfaces and may be someone has some > > cons > > > >>>>> or suggestions? > > > >>>>> > > > >>>>> On Fri, Feb 3, 2017 at 5:31 AM, Valentin Kulichenko < > > > >>>>> valentin.kuliche...@gmail.com> wrote: > > > >>>>> > > > >>>>>> Andrey, > > > >>>>>> > > > >>>>>> In which case compatibility is broken? If there is a method that > > > >>>> returns > > > >>>>>> void and you change it to return some type, it doesn't break > > > >>> anything, > > > >>>>>> because currently nobody can assign the result of this method > to a > > > >>>>>> variable. I.e. in old code the returned value will be always > > > ignored, > > > >>>>>> therefore it can be of any type. > > > >>>>>> > > > >>>>>> Is there anything else that I'm missing? > > > >>>>>> > > > >>>>>> -Val > > > >>>>>> > > > >>>>>> On Thu, Feb 2, 2017 at 3:49 AM, Andrey Mashenkov < > > > >>>>>> andrey.mashen...@gmail.com > > > >>>>>>> wrote: > > > >>>>>> > > > >>>>>>> Hi Igniters, > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> I'm working on IGNITE-4564 [1] to make our configuration > classes > > > >>> and > > > >>>>> SPI > > > >>>>>>> classes more convenient. > > > >>>>>>> > > > >>>>>>> There is no problem to change return type in setter method > > > >>> signatures > > > >>>>>>> and override methods in child child classes to make them return > > > >>> more > > > >>>>>>> accurate type. > > > >>>>>>> > > > >>>>>>> But, I found that we have set methods in some of our interfaces > > and > > > >>>>>>> changing its 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
