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/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? >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>> >>>> >> >>
