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 <dma...@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 <ptupit...@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.goncha...@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 <voze...@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 <voze...@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 <ptupit...@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.chugu...@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 < > >>> voze...@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 < > >>> ptupit...@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? > >>>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>>> > >>> > >> > >
