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