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.goncha...@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 <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? > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > >
