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