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