Sergey, We need to keep API consistent. If we usually return bytes, then these metrics should return bytes as well. What is more important, when I look at API I understand that this thing is not metrics at all. Metrics in Ignte terms is a set of read-only numeric properties. But this interface contains strange things like "name", "swapPilePath". What even more strange, I do not see how to get these metrics from public API.
All in all, looks like this entity is not "metrics", but "MBean" in usual Ignite terms. Vladimir. On Mon, Apr 24, 2017 at 7:20 PM, Pavel Tupitsyn <ptupit...@apache.org> wrote: > Sergey, I disagree. > > 1) As a user, I would expect MemoryMetrics instance to be > read-only and serializable, so I can send it somewhere, store, > put into a collection and draw a graph in UI, etc. > > Other metrics APIs allow this, MemoryMetrics does not. > > 2) Methods like enableMetrics and rateTimeInterval placed in MemoryMetrics > break single responsibility principle and are confusing. > Why do I need to Get metrics to Enable them? > > These methods should be placed somewhere else. > > I would suggest some thing like this: > - introduce Memory class with getMetrics, enableMetrics, > setRateTimeInterval, etc > - add Ignite.getMemory method > > > On Mon, Apr 24, 2017 at 6:53 PM, Sergey Chugunov < > sergey.chugu...@gmail.com> > wrote: > > > I guess the main reason to have IgniteCache returning snapshot metrics > was > > to collect metrics about distributed cache across the cluster. > > At the same time MemoryMetrics were initially designed to be local on > each > > node, there were no requirements about collecting cluster-wide > > MemoryMetrics. > > > > Collecting MemoryMetrics is considered as an investigation action when > > something goes wrong, that's why it was decided to add enable/disable > > actions to the interface. > > So for me it sounds reasonable. > > > > The only debatable thing is having MemoryMetrics returning size in > > megabytes, however I think about these metrics as designed only for user, > > so I think it makes sense to return this metric in human-readable form. > > > > On Mon, Apr 24, 2017 at 6:41 PM, Vladimir Ozerov <voze...@gridgain.com> > > wrote: > > > > > Agree, looks inconsistent to me. > > > > > > Alex G., could you chime in? > > > > > > On Mon, Apr 24, 2017 at 6:30 PM, Pavel Tupitsyn <ptupit...@apache.org> > > > wrote: > > > > > > > Igniters, > > > > > > > > I have noticed quite a lot of inconsistencies with the rest of our > APIs > > > in > > > > our new MemoryMetrics API: > > > > > > > > 1) MemoryMetrics is not a snapshot. It is a "live" instance which > > returns > > > > different values each time you access some property. > > > (IgniteCache.metrics() > > > > returns a snapshot). > > > > > > > > 2) MemoryMetrics has methods that modify state, like enableMetrics > > > > and rateTimeInterval > > > > (IgniteCache.metrics() returns a read-only serializable snapshot) > > > > > > > > 3) getSize method returns size in megabytes > > > > (IgniteCache.metrics() provides sizes in bytes) > > > > > > > > > > > > I propose to rework this API until it is not too late. > > > > > > > > Thoughts? > > > > > > > > > >
