ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrey Mashenkov <andrey.mashen...@gmail.com>
Subject Re: Ensure that builder approach is used for all setters in public API
Date Thu, 09 Feb 2017 14:09:36 GMT
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 <yzhdanov@apache.org> wrote:

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



-- 
Best regards,
Andrey V. Mashenkov

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