Agree. -Val
On Fri, Dec 29, 2017 at 3:08 PM, Denis Magda <dma...@apache.org> wrote: > Now I see. Seems I was doing something wrong in my initial reproducer. > > Updated cache metrics readme doc by purging any events related parameters > from there: > https://apacheignite.readme.io/v2.3/docs/cache-metrics < > https://apacheignite.readme.io/v2.3/docs/cache-metrics> > > The events readme doc looks good to me. Just updated the javadoc of > IgniteEvents#*Query methods to bring MemoryEventStorageSpi to users > attention. > > As for the cache events, it’s an oversight that there is no parameter that > enables/disables the events per cache. Let’s add > CacheConfiguration.setEventsEnabled > method and have it enabled by default (current behavior). If the user > deploys hundreds of caches or just interested in the events of specific > ones then he can always set this property to ‘false’ in configuration of > the caches of no interest: > https://issues.apache.org/jira/browse/IGNITE-7346 < > https://issues.apache.org/jira/browse/IGNITE-7346> > > Agree? > > — > Denis > > > > > On Dec 29, 2017, at 11:10 AM, Valentin Kulichenko < > valentin.kuliche...@gmail.com> wrote: > > > > Guys, > > > > I'm not sure what issue we're trying to solve. Basically, we have three > > different functionality parts here: > > > > 1. Cache metrics exposed via CacheMetrics interface and MBeans (number of > > puts, average put time, this kind of stuff). These are controlled on per > > cache level by CacheConfiguration#statisticsEnabled property. > > 2. Listening to cache events. To achieve this you only need to enable > > particular event types and register listeners, this does not depend on > > CacheConfiguration#statisticsEnabled. Here is the test confirming this: > > https://gist.github.com/vkulichenko/54043c7b75c69eca5515e7d7692b5276 > > 3. Querying cache events via IgniteEvents#*Query methods. This is the > ONLY > > piece that requires MemoryEventStorageSpi to be configured. Since > querying > > is not very frequently used, and event storage introduces significant > > memory overhead, I don't think we should ever implicitly enable it. We > > deliberately made no-op implementation the default one not so long ago. > > > > All three points above seem to work without any issues. The only useful > > addition I see here is ability to enable cache events on per cache level. > > But for this we can introduce new property to CacheConfiguration. I would > > not mix metrics and events together as these are different APIs designed > > for different purposes. > > > > Am I missing something? > > > > -Val > > > > On Fri, Dec 29, 2017 at 8:02 AM, Denis Magda <dma...@apache.org> wrote: > > > >> Alexey, > >> > >> I think we should enable memoryEventStorageSPI automatically whenever > >> statisticsEnabled is toggled on. A special message has to be added to > the > >> log pointing out that the automatic activation happened. > >> > >> Does it sound like a good solution? > >> > >> — > >> Denis > >> > >>> On Dec 29, 2017, at 3:51 AM, Alexey Plekhanov <plehanov.a...@gmail.com > > > >> wrote: > >>> > >>> Denis, I start working on the issue > >>> https://issues.apache.org/jira/browse/IGNITE-6925 and now I don't > >> understand > >>> why these properties must be bound to each other? > >>> > >>> • If we enable statistics on caches, this does not necessarily mean, > >> that > >>> we wanted to get some events, we can enable statistics for other > reasons. > >>> Conversely, not all events need statistics to be enabled on caches. So > we > >>> can’t bind statistics flag to events (subscribe to events when > >> statistics is > >>> enabled or enable statistics, when subscribing to events) > >>> • We can’t set events of interest, when we set not a dummy > >> EventsStorageSpi, > >>> because we don’t know which events are interesting. > >>> • When we set events of interest, it’s not necessary, that these events > >> will > >>> be monitored using EventsStorageSpi, we can also subscribe to events by > >>> events listeners, in this case EventsStorageSpi don’t used. > >>> > >>> So, there is no general rule (if ... enabled, then ... must be enabled > >> too). > >>> The only implementation I can propose - is "set MemoryEventStorageSPI > >>> instead of NoopEventStorageSPI when includeEventTypes list is not > empty", > >>> but even this implementation may be warranted only in some cases. > >>> > >>> > >>> Denis Magda-2 wrote > >>>> Let’s simplifying the metrics as a part of this ticket: > >>>> https://issues.apache.org/jira/browse/IGNITE-5796 > >>>> <https://issues.apache.org/jira/browse/IGNITE-5796> > >>>> > >>>> Expanded its scope. > >>>> > >>>> — > >>>> Denis > >>>> > >>>>> On Sep 9, 2017, at 2:44 PM, Valentin Kulichenko < > >>> > >>>> valentin.kulichenko@ > >>> > >>>> > wrote: > >>>>> > >>>>> statisticsEnabled property comes from JCache, BTW. > >>>>> > >>>>> -Val > >>>>> > >>>>> On Sat, Sep 9, 2017 at 11:09 AM, Dmitriy Setrakyan < > >>> > >>>> dsetrakyan@ > >>> > >>>> > > >>>>> wrote: > >>>>> > >>>>>> On Sat, Sep 9, 2017 at 8:56 AM, Denis Magda < > >>> > >>>> dmagda@ > >>> > >>>> > wrote: > >>>>>> > >>>>>>> Surprise! > >>>>>>> > >>>>>>> If you want to see cache events then you have to enable one more > >> flag! > >>>>>>> > >>>>>>> > >>>> <property name="StatisticsEnabled" value="true"/> > >>>>>> > >>>>>> > >>>>>> What is the overhead of this statistics collection? > >>>>>> > >>>>>> > >>>>>>> Three flags/beans have to be in the config in total, three! Just to > >> see > >>>>>>> cache events. The API is a mess. Let’s contemplate how to fix it. > >>>>>> > >>>>>> > >>>>>> Agree, this is horrible. We need to fix it in 2.3. Is there a > ticket? > >>> > >>> > >>> > >>> > >>> > >>> -- > >>> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ > >> > >> > >
