ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sergey Chugunov <sergey.chugu...@gmail.com>
Subject Re: MemoryMetrics interface inconsistencies
Date Thu, 27 Apr 2017 12:36:41 GMT
Denis,

I added javadoc for MemoryMetrics in the commit cee78f47
<https://github.com/apache/ignite/commit/cee78f47d132001f56c749103f2d8ff7e6c56aa2>
 in ignite-5072-merge
<https://github.com/apache/ignite/compare/ignite-5072-merge> branch.

Could you please review it? I would like to have a feedback and improve
documentation if necessary.

Thanks,
Sergey

On Wed, Apr 26, 2017 at 8:08 PM, Denis Magda <dmagda@apache.org> wrote:

> Sergey,
>
> > There is no such thing as a "whole page memory", just look at the code.
> > PageMemoryNoStoreImpl in AI manages single memory region (although
> > expandable); and each MemoryPolicy has one and only one
> > PageMemoryNoStoreImpl instance associated with it.
>
>
> Under the “whole page memory” I meant all the regions defined by all the
> memory policies set up. I still think that it’s reasonable to have the
> metrics for the “whole page memory” so that people can see total memory
> consumption, etc. However, let’s put this off till 2.1. This should be an
> easy thing to add in the future.
>
> > One more thing I cannot avoid mentioning is that the whole situation
> around
> > interface inconsistency raised from huge lacks in documentation about
> > metrics in AI in general. I haven't found any javadocs or wikis
> describing
> > overall architecture of metrics gathering in Ignite, how they are exposed
> > to users or what are use cases. And I still don't have a full picture of
> > this.
>
> Yes, there is a gap in here. We will start improving the situation with
> this ticket:
> https://issues.apache.org/jira/browse/IGNITE-4963 <
> https://issues.apache.org/jira/browse/IGNITE-4963>
>
> —
> Denis
>
> > On Apr 26, 2017, at 7:07 AM, Sergey Chugunov <sergey.chugunov@gmail.com>
> wrote:
> >
> > Denis,
> >
> > There is no such thing as a "whole page memory", just look at the code.
> > PageMemoryNoStoreImpl in AI manages single memory region (although
> > expandable); and each MemoryPolicy has one and only one
> > PageMemoryNoStoreImpl instance associated with it.
> >
> > Each MemoryMetricsImpl is aimed to collect metrics from one MemoryPolicy
> > (allocations, evictions, etc.) on local node. I'll reflect this in
> > documentation for sure and ask you and other people to review it.
> >
> > One more thing I cannot avoid mentioning is that the whole situation
> around
> > interface inconsistency raised from huge lacks in documentation about
> > metrics in AI in general. I haven't found any javadocs or wikis
> describing
> > overall architecture of metrics gathering in Ignite, how they are exposed
> > to users or what are use cases. And I still don't have a full picture of
> > this.
> > So if there are any experts who know more than me, I think it is a must
> for
> > them to share their knowledge with community somewhere in javadoc or
> other
> > easily accessible place.
> >
> > Thanks,
> > Sergey.
> >
> > On Tue, Apr 25, 2017 at 8:46 PM, Denis Magda <dmagda@apache.org> wrote:
> >
> >> Totally agree with all Pavel’s and Vovan's suggestions and concerns,
> >> especially, with the following:
> >>
> >>> 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)
> >>
> >> In general, it’s unclear how to use it and the javadoc doesn’t explain
> >> whether these are the metrics for a pool/region/block defined by memory
> >> policy or the whole page memory available to a node will be monitored.
> >> Please mention all that in the javadoc and don’t refer to internal
> classes
> >> like PageMemory and FreeList the documentation.
> >>
> >> —
> >> Denis
> >>
> >>> On Apr 25, 2017, at 12:23 AM, Pavel Tupitsyn <ptupitsyn@apache.org>
> >> wrote:
> >>>
> >>> 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