ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Denis Magda <dma...@apache.org>
Subject Re: MemoryMetrics interface inconsistencies
Date Wed, 26 Apr 2017 17:08:24 GMT
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.chugunov@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 <dmagda@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 <ptupitsyn@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.goncharuk@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 <vozerov@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 <vozerov@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 <ptupitsyn@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.chugunov@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 <
>>>>> vozerov@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 <
>>>>> ptupitsyn@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?
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>> 
>> 


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message