ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexey Goncharuk <alexey.goncha...@gmail.com>
Subject Re: MemoryMetrics interface inconsistencies
Date Mon, 24 Apr 2017 17:06:19 GMT
Agree, I overlooked this during the review. However, the changes to fix
this is pretty simple - we just move all mutator methods to MBean, like
Vladimir suggested and make MBean return the live data, while the public
API will return a serializable snapshot.

Agreed?

2017-04-24 19:33 GMT+03:00 Vladimir Ozerov <vozerov@gridgain.com>:

> It seems that all mutator methods should be simply moved to MBean interface
> and change MBytes to bytes. In this case metrics interface will be
> consistent.
>
> On Mon, Apr 24, 2017 at 7:29 PM, Vladimir Ozerov <vozerov@gridgain.com>
> wrote:
>
> > Sergey,
> >
> > We need to keep API consistent. If we usually return bytes, then these
> > metrics should return bytes as well. What is more important, when I look
> at
> > API I understand that this thing is not metrics at all. Metrics in Ignte
> > terms is a set of read-only numeric properties. But this interface
> contains
> > strange things like "name", "swapPilePath". What even more strange, I do
> > not see how to get these metrics from public API.
> >
> > All in all, looks like this entity is not "metrics", but "MBean" in usual
> > Ignite terms.
> >
> > Vladimir.
> >
> > On Mon, Apr 24, 2017 at 7:20 PM, Pavel Tupitsyn <ptupitsyn@apache.org>
> > wrote:
> >
> >> Sergey, I disagree.
> >>
> >> 1) As a user, I would expect MemoryMetrics instance to be
> >> read-only and serializable, so I can send it somewhere, store,
> >> put into a collection and draw a graph in UI, etc.
> >>
> >> Other metrics APIs allow this, MemoryMetrics does not.
> >>
> >> 2) Methods like enableMetrics and rateTimeInterval placed in
> MemoryMetrics
> >> break single responsibility principle and are confusing.
> >> Why do I need to Get metrics to Enable them?
> >>
> >> These methods should be placed somewhere else.
> >>
> >> I would suggest some thing like this:
> >> - introduce Memory class with getMetrics, enableMetrics,
> >> setRateTimeInterval, etc
> >> - add Ignite.getMemory method
> >>
> >>
> >> On Mon, Apr 24, 2017 at 6:53 PM, Sergey Chugunov <
> >> sergey.chugunov@gmail.com>
> >> wrote:
> >>
> >> > I guess the main reason to have IgniteCache returning snapshot metrics
> >> was
> >> > to collect metrics about distributed cache across the cluster.
> >> > At the same time MemoryMetrics were initially designed to be local on
> >> each
> >> > node, there were no requirements about collecting cluster-wide
> >> > MemoryMetrics.
> >> >
> >> > Collecting MemoryMetrics is considered as an investigation action when
> >> > something goes wrong, that's why it was decided to add enable/disable
> >> > actions to the interface.
> >> > So for me it sounds reasonable.
> >> >
> >> > The only debatable thing is having MemoryMetrics returning size in
> >> > megabytes, however I think about these metrics as designed only for
> >> user,
> >> > so I think it makes sense to return this metric in human-readable
> form.
> >> >
> >> > On Mon, Apr 24, 2017 at 6:41 PM, Vladimir Ozerov <
> vozerov@gridgain.com>
> >> > wrote:
> >> >
> >> > > Agree, looks inconsistent to me.
> >> > >
> >> > > Alex G., could you chime in?
> >> > >
> >> > > On Mon, Apr 24, 2017 at 6:30 PM, Pavel Tupitsyn <
> ptupitsyn@apache.org
> >> >
> >> > > wrote:
> >> > >
> >> > > > Igniters,
> >> > > >
> >> > > > I have noticed quite a lot of inconsistencies with the rest of
our
> >> APIs
> >> > > in
> >> > > > our new MemoryMetrics API:
> >> > > >
> >> > > > 1) MemoryMetrics is not a snapshot. It is a "live" instance which
> >> > returns
> >> > > > different values each time you access some property.
> >> > > (IgniteCache.metrics()
> >> > > > returns a snapshot).
> >> > > >
> >> > > > 2) MemoryMetrics has methods that modify state, like enableMetrics
> >> > > > and rateTimeInterval
> >> > > > (IgniteCache.metrics() returns a read-only serializable snapshot)
> >> > > >
> >> > > > 3) getSize method returns size in megabytes
> >> > > > (IgniteCache.metrics() provides sizes in bytes)
> >> > > >
> >> > > >
> >> > > > I propose to rework this API until it is not too late.
> >> > > >
> >> > > > Thoughts?
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>

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