ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Setrakyan <dsetrak...@apache.org>
Subject Re: MemoryMetrics interface inconsistencies
Date Mon, 01 May 2017 06:02:16 GMT
This thread has been going on for a while. Where can I see the final design
proposal?

On Thu, Apr 27, 2017 at 10:26 AM, Denis Magda <dmagda@apache.org> wrote:

> Sergey, thanks,
>
> I’ve reviewed and merged corrections directly into ignite-5072-merge.
> Don’t miss them when the branch will be synched up with ignite-2.0.
>
> However, these are some of the opened questions:
> * Ignite.memoryMetrics() returns a collection of metrics. However, it’s
> unclear what exactly the collection includes. Is every element of the
> collection is a latest snapshot of a specific memory region defined by
> memory policy and the the total size of the collection is equal to the
> total number of such regions configured on a node? This has to be clarified
> in java doc before 2.0 release.
>
> * I would have Ignite.memoryMetrics(name) methods that will return the
> metrics for a concrete region configured by memory policy. It’s fine to add
> the method under 2.1 (create a ticket).
>
> * I would add “setRateInterval” and “setSubIntervals” methods to
> MemoryPolicyConfiguration because now I have to get a JMX bean if to want
> to change the parameter. Again, feel free do this under 2.0 or move to 2.1
> if there is no time for that.
>
> —
> Denis
>
> > On Apr 27, 2017, at 5:36 AM, Sergey Chugunov <sergey.chugunov@gmail.com>
> wrote:
> >
> > Denis,
> >
> > I added javadoc for MemoryMetrics in the commit cee78f47
> > <https://github.com/apache/ignite/commit/cee78f47d132001f56c749103f2d8f
> f7e6c56aa2>
> > 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