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? > > > > > >
