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