You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Anton Vinogradov <av...@gridgain.com> on 2018/02/01 09:46:48 UTC
Re: Annoying extra steps for enabling metrics
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.kulichenko@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 <pl...@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 <pl...@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 <dm...@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.kulichenko@gmail.com> wrote:
> > >> >
> > >> > Agree.
> > >> >
> > >> > -Val
> > >> >
> > >> > On Fri, Dec 29, 2017 at 3:08 PM, Denis Magda <dm...@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.kulichenko@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 <dm...@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.alex@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/
> > >> >>>>
> > >> >>>>
> > >> >>
> > >> >>
> > >>
> > >>
> > >
> >
>
Re: Annoying extra steps for enabling metrics
Posted by Alex Plehanov <pl...@gmail.com>.
Ok, then I will rename methods to setEventsDisabled/getEventsDisabled.
2018-02-01 12:46 GMT+03:00 Anton Vinogradov <av...@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.kulichenko@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 <pl...@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 <pl...@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 <dm...@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.kulichenko@gmail.com> wrote:
> > > >> >
> > > >> > Agree.
> > > >> >
> > > >> > -Val
> > > >> >
> > > >> > On Fri, Dec 29, 2017 at 3:08 PM, Denis Magda <dm...@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.kulichenko@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 <dmagda@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.alex@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/
> > > >> >>>>
> > > >> >>>>
> > > >> >>
> > > >> >>
> > > >>
> > > >>
> > > >
> > >
> >
>