ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yakov Zhdanov <yzhda...@apache.org>
Subject Re: Ensure that builder approach is used for all setters in public API
Date Wed, 08 Feb 2017 04:23:27 GMT
+1 to Vladimir suggestion

--Yakov

2017-02-07 20:50 GMT+07:00 Vladimir Ozerov <vozerov@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.mashenkov@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.kulichenko@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.mashenkov@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 <
> vozerov@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.kulichenko@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 <dmagda@apache.org>
> > > > wrote:
> > > > > >
> > > > > > > Sorry, “public modifications” -> “public APIs”
> > > > > > >
> > > > > > > —
> > > > > > > Denis
> > > > > > >
> > > > > > > > On Feb 3, 2017, at 10:03 AM, Denis Magda <dmagda@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.mashenkov@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
<
> > > > > > vozerov@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.kulichenko@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.mashenkov@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.kulichenko@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.mashenkov@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
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message