ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pavel Tupitsyn <ptupit...@apache.org>
Subject Re: MemoryMetrics interface inconsistencies
Date Tue, 25 Apr 2017 07:23:59 GMT
Can you give an example of such API usage?
A piece of code to enable metrics and retrieve a snapshot?

On Mon, Apr 24, 2017 at 8:06 PM, Alexey Goncharuk <
alexey.goncharuk@gmail.com> wrote:

> 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