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 Mon, 01 May 2017 17:57:40 GMT
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 <dsetrakyan@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 <dmagda@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.chugunov@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 <dmagda@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.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
View raw message