You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Pavel Tupitsyn <pt...@apache.org> on 2017/04/24 15:30:50 UTC
MemoryMetrics interface inconsistencies
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?
Re: MemoryMetrics interface inconsistencies
Posted by Denis Magda <dm...@apache.org>.
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 <ds...@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 <dm...@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 <se...@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 <dm...@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 <dm...@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 <pt...@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 <vo...@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?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
Re: MemoryMetrics interface inconsistencies
Posted by Dmitriy Setrakyan <ds...@apache.org>.
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 <dm...@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 <se...@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 <dm...@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 <dm...@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 <pt...@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 <vo...@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?
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>
> >>>>
> >>
> >>
>
>
Re: MemoryMetrics interface inconsistencies
Posted by Denis Magda <dm...@apache.org>.
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 <se...@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 <dm...@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 <se...@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 <dm...@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 <pt...@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 <vo...@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?
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
Re: MemoryMetrics interface inconsistencies
Posted by Sergey Chugunov <se...@gmail.com>.
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 <dm...@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 <se...@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 <dm...@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 <pt...@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 <vo...@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?
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
> >>
>
>
Re: MemoryMetrics interface inconsistencies
Posted by Denis Magda <dm...@apache.org>.
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 <se...@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 <dm...@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 <pt...@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 <vo...@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?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>
>>
Re: MemoryMetrics interface inconsistencies
Posted by Sergey Chugunov <se...@gmail.com>.
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 <dm...@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 <pt...@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 <vo...@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?
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>>
> >>>
> >>
>
>
Re: MemoryMetrics interface inconsistencies
Posted by Denis Magda <dm...@apache.org>.
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 <pt...@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 <vo...@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 <vo...@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 <pt...@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?
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
Re: MemoryMetrics interface inconsistencies
Posted by Pavel Tupitsyn <pt...@apache.org>.
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 <vo...@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 <vo...@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 <pt...@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?
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> > >
> >
>
Re: MemoryMetrics interface inconsistencies
Posted by Alexey Goncharuk <al...@gmail.com>.
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 <vo...@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 <vo...@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 <pt...@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?
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>
Re: MemoryMetrics interface inconsistencies
Posted by Vladimir Ozerov <vo...@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 <vo...@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 <pt...@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 <vo...@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?
>> > > >
>> > >
>> >
>>
>
>
Re: MemoryMetrics interface inconsistencies
Posted by Vladimir Ozerov <vo...@gridgain.com>.
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 <pt...@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 <vo...@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 <pt...@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?
> > > >
> > >
> >
>
Re: MemoryMetrics interface inconsistencies
Posted by Pavel Tupitsyn <pt...@apache.org>.
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 <se...@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 <vo...@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 <pt...@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?
> > >
> >
>
Re: MemoryMetrics interface inconsistencies
Posted by Sergey Chugunov <se...@gmail.com>.
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 <vo...@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 <pt...@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?
> >
>
Re: MemoryMetrics interface inconsistencies
Posted by Vladimir Ozerov <vo...@gridgain.com>.
Agree, looks inconsistent to me.
Alex G., could you chime in?
On Mon, Apr 24, 2017 at 6:30 PM, Pavel Tupitsyn <pt...@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?
>