Ok, then I will rename methods to setEventsDisabled/getEventsDisabled.
2018-02-01 12:46 GMT+03:00 Anton Vinogradov <avinogra...@gridgain.com>: > Folks, > > .setEventsDisabled looks to be a good solution, since we will always call > it like .setEventsDisabled(true). > > Calling .setEventsEnabled(false), since true is a default, looks odd to me. > > On Wed, Jan 31, 2018 at 11:02 PM, Valentin Kulichenko < > valentin.kuliche...@gmail.com> wrote: > > > Alex, > > > > As a workaround, you can use boxed Boolean as a field type, and then > return > > true from getEventsEnabled in case it's null. Will this work? > > > > -Val > > > > On Wed, Jan 31, 2018 at 7:31 AM, Alex Plehanov <plehanov.a...@gmail.com> > > wrote: > > > > > Denis, there is a question about IGNITE-7346. > > > > > > CacheConfiguration fields are set to they default values when cache > > > configuration is created by constructor. When CacheConfiguration is > > created > > > by deserialization (from another node or from PDS), constructor is not > > > called. If it was serialized by older version of Ignite, a new added > > > boolean fields are set to false after deserialization by a new Ignite > > > versions. We can't change this behavior without methods for custom > > > serialization/deserialization (which are not implemented now in > > > CacheConfiguration class). It will differ from requirements for "Eve > > > ntsEnabled" property (events must be enabled for caches by default). > > > > > > I think we can reverse the logic and rename method > > > CacheConfiguration.setEventsEnabled > > > to CacheConfiguration.setEventsDisabled, in this case the field value > > will > > > always be "false" by default. > > > > > > What do you think about it? > > > > > > 2017-12-30 10:12 GMT+03:00 Alex Plehanov <plehanov.a...@gmail.com>: > > > > > > > Due to holidays I can start work on this ticket only after 8 jan 2018 > > > > > > > > 2017-12-30 2:12 GMT+03:00 Denis Magda <dma...@apache.org>: > > > > > > > >> Good, closed the original ticket. > > > >> > > > >> Alex P, do you have time to work on IGNITE-7346 instead to address > the > > > >> issue with the cache events per cache in 2.4 release? > > > >> > > > >> — > > > >> Denis > > > >> > > > >> > On Dec 29, 2017, at 3:10 PM, Valentin Kulichenko < > > > >> valentin.kuliche...@gmail.com> wrote: > > > >> > > > > >> > 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/ > > 54043c7b75c69eca5515e7d7692b52 > > > 76 > > > >> >>> 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/ > > > >> >>>> > > > >> >>>> > > > >> >> > > > >> >> > > > >> > > > >> > > > > > > > > > >
