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