+1 to Vladimir suggestion --Yakov
2017-02-07 20:50 GMT+07:00 Vladimir Ozerov <voze...@gridgain.com>: > Andrey, Valya, > > There is another problem here. What is we decide to add some existing > setter method to MBean? If it has signature "T setSomething(...)", we will > not be able to do so. We need to understand how to deal with it, so that > possible further improvements to MBean-s are not compromised. Any ideas? > May be we should fully decouple MBeans into separate classes? > > E.g. instead of: > FifoEvictionPolicy implements FifoEvictionPolicyMBean > > we will have > FifoEvictionPolicy > FifoEvictionPolicyMBeanImpl implements FifoEvictionPolicyMBean > > This way public API will be fully decoupled form JMX what seems reasonable > to me. Thoughts? > > On Tue, Feb 7, 2017 at 4:31 PM, Andrey Mashenkov < > andrey.mashen...@gmail.com > > wrote: > > > Val, > > > > void setBatchSize(int batchSize) > > void setMaxMemorySize(long maxMemSize) > > void setMaxSize(int max) > > void setExcludePaths(Collection<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 < > > valentin.kuliche...@gmail.com> wrote: > > > > > Andrey, > > > > > > Can you list all setters that we have on MBeans? > > > > > > -Val > > > > > > On Mon, Feb 6, 2017 at 2:21 PM, Andrey Mashenkov < > > > andrey.mashen...@gmail.com > > > > wrote: > > > > > > > Folks, > > > > > > > > Changing MBeans setters signature is bad idea. AOP tests failed on TC > > > with > > > > this change. > > > > > > > > On Mon, Feb 6, 2017 at 11:21 AM, Vladimir Ozerov < > voze...@gridgain.com > > > > > > > wrote: > > > > > > > > > Val, > > > > > > > > > > Good catch! Can we try leaving SPIs and base methods untouched? > Will > > it > > > > API > > > > > be consistent in this case? > > > > > > > > > > On Fri, Feb 3, 2017 at 10:22 PM, Valentin Kulichenko < > > > > > valentin.kuliche...@gmail.com> wrote: > > > > > > > > > > > Folks, > > > > > > > > > > > > I tend to think that the problem is that we try to apply 'builder > > > > > approach' > > > > > > to *ALL* setters. Let's approach this smarter. > > > > > > > > > > > > This approach is actually applicable only for configuration > setters > > > > > > available on public API, i.e. it's only about setters on > > > > ***Configuration > > > > > > classes and SPI *implementations*. For SPI interface methods like > > > > > > 'CollisionSpi.setExternalCollisionListener' this makes no > sense, I > > > > would > > > > > > not touch them. > > > > > > > > > > > > The only thing I still don't like is MBeans. Returning something > > > except > > > > > > void on MBean interfaces look ugly, but without doing this we > will > > > > break > > > > > > API consistency on the implementation. Any ideas on how to > approach > > > > this? > > > > > > > > > > > > -Val > > > > > > > > > > > > On Fri, Feb 3, 2017 at 10:29 AM, Denis Magda <dma...@apache.org> > > > > wrote: > > > > > > > > > > > > > Sorry, “public modifications” -> “public APIs” > > > > > > > > > > > > > > — > > > > > > > Denis > > > > > > > > > > > > > > > On Feb 3, 2017, at 10:03 AM, Denis Magda <dma...@apache.org> > > > > wrote: > > > > > > > > > > > > > > > > Andrey, > > > > > > > > > > > > > > > > If the changes affect public modifications don’t forget to > > update > > > > > this > > > > > > > page: > > > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/ > > > > > > > Apache+Ignite+2.0+Migration+Guide <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 > > > > > > > > > > > > > > > -- > > Best regards, > > Andrey V. Mashenkov > > >
