All the outstanding issues and concerns were addressed. You can look at the current design by taking the 2.0 release candidate https://dist.apache.org/repos/dist/dev/ignite/2.0.0-rc2/ and locating MemoryMetrics.html in the “docs” folder.
Minor issues will be handled in 2.1: https://issues.apache.org/jira/browse/IGNITE-5124 — Denis > On Apr 30, 2017, at 11:02 PM, Dmitriy Setrakyan <dsetrak...@apache.org> wrote: > > 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? >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>> >>>>>> >>>> >>>> >> >>
