geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Oleske <mole...@pivotal.io>
Subject Re: Should Geode stats conform to backwards compatibility constraints?
Date Thu, 14 Feb 2019 21:35:58 GMT
Here's a PR for a revert of the offending commits.

https://github.com/apache/geode/pull/3195

Seems valuable to revert for now and decide a path forward rather than rush
to get in 1.9

-michael

On Wed, Feb 13, 2019 at 9:51 PM Jacob Barrett <jbarrett@pivotal.io> wrote:

> How about mod MAX_INT? It would wrap back to 0 and make it more consistent
> with at least SNMP counters that roll over to 0 when maxed. A monitoring
> and graphing system can account for this by recognizing the current value
> is less than the previous and typically uses the previous and current
> values to recreate the true value.
>
> -Jake
>
>
> > On Feb 13, 2019, at 8:22 PM, Ryan McMahon <rmcmahon@pivotal.io> wrote:
> >
> > Sorry that should read “and if the value exceeds MAX_INT”
> >
> >> On Wed, Feb 13, 2019 at 8:21 PM Ryan McMahon <rmcmahon@pivotal.io>
> wrote:
> >>
> >> +1 to introducing a new method which returns the stat as long per Jake’s
> >> suggestion.  I vote for getInt() to downcast as an int, and the value
> >> exceeds MAX_INT, return MAX_INT and possibly add a warning message which
> >> points users to the new long version of the method.  I think throwing an
> >> exception might be a bit much personally.
> >>
> >> Ryan
> >>
> >>> On Wed, Feb 13, 2019 at 4:24 PM Owen Nichols <onichols@pivotal.io>
> wrote:
> >>>
> >>> Is it possible for the underlying counter to be maintained as a long,
> and
> >>> change the getInt method to return as normal when the value is within
> >>> MAX_INT, otherwise throw an exception and/or return MAX_INT when the
> long
> >>> value would overflow an int?
> >>>
> >>> For the MX Bean, should we keep (but deprecate) the existing attribute,
> >>> and add a new attribute TotalNetSearchCompletedAsLong?
> >>>
> >>>> On Feb 13, 2019, at 3:58 PM, Kirk Lund <klund@apache.org> wrote:
> >>>>
> >>>> Quite a few Geode stats are currently defined as IntCounters which
> very
> >>>> easily wrap past Integer.MAX_VALUE. Some users wanted these stats to
> not
> >>>> wrap to negative MAX_VALUE, so my team defined the following two
> tickets
> >>>> and changed them to LongCounters on the develop branch:
> >>>>
> >>>> GEODE-6345: StatSamplerStats jvmPauses stat may wrap to negative value
> >>>> https://issues.apache.org/jira/browse/GEODE-6345
> >>>>
> >>>> GEODE-6334: CachePerfStats operation count stats may wrap to negative
> >>> values
> >>>> https://issues.apache.org/jira/browse/GEODE-6334
> >>>>
> >>>> Some people are now concerned that this may break backwards
> >>> compatibility
> >>>> and API for existing users.
> >>>>
> >>>> There are two potential ways for a user to *experience* this as an API
> >>>> change:
> >>>>
> >>>> 1) MemberMXBean in JMX
> >>>>
> >>>> *-  int getTotalNetSearchCompleted();*
> >>>> *+  long getTotalNetSearchCompleted();*
> >>>>
> >>>> As you can see in GEODE-6334, we changed quite a few stats from
> integer
> >>> to
> >>>> long in CachePerfStats. The only one directly exposed as an attribute
> on
> >>>> MemberMXBean is TotalNetSearchCompleted.
> >>>>
> >>>> Users will find that this API method changed.
> >>>>
> >>>> 2) Statistics API with undocumented strings
> >>>>
> >>>> If we assume that users might know or discover that we have a
> statistics
> >>>> text id of "cachePerfStats" with an integer stat of name "puts" then
> >>> they
> >>>> could use our Statistics API to get the value:
> >>>>
> >>>> * 1:    CacheFactory cacheFactory = new CacheFactory();*
> >>>> * 2:    Cache cache = cacheFactory.create();*
> >>>> * 3:*
> >>>> * 4:    Region<String, String> region = cache.<String,
> >>>> String>createRegionFactory(PARTITION).create("region");*
> >>>> * 5:*
> >>>> * 6:    Statistics[] statistics =
> >>>>
> cache.getDistributedSystem().findStatisticsByTextId("cachePerfStats");*
> >>>> * 7:    int oldPuts = statistics[0].getInt("puts");*
> >>>> * 8:*
> >>>> * 9:    region.put("some", "value");*
> >>>> *10:*
> >>>> *11:    int newPuts = statistics[0].getInt("puts");*
> >>>> *12:*
> >>>> *13:    assertThat(newPuts).isEqualTo(oldPuts + 1);*
> >>>>
> >>>> The above works in Geode 1.8 and earlier but will begin to throw the
> >>>> following in Geode 1.9 (when develop branch is released):
> >>>>
> >>>> *java.lang.IllegalArgumentException: The statistic puts with id 23 is
> of
> >>>> type long and it was expected to be an int.*
> >>>> * at
> >>>>
> >>>
> org.apache.geode.internal.statistics.StatisticDescriptorImpl.checkInt(StatisticDescriptorImpl.java:324)*
> >>>> * at
> >>>>
> >>>
> org.apache.geode.internal.statistics.StatisticsImpl.getIntId(StatisticsImpl.java:577)*
> >>>> * at
> >>>>
> >>>
> org.apache.geode.internal.statistics.StatisticsImpl.getInt(StatisticsImpl.java:274)*
> >>>> * at
> >>>>
> >>>
> org.apache.geode.internal.statistics.StatisticsImpl.getInt(StatisticsImpl.java:269)*
> >>>> * at com.user.example.Example.example(Example.java7)*
> >>>>
> >>>> In order to avoid the above exception, a user would have to change the
> >>> code
> >>>> on lines 7 and 11 to use *getLong* instead of *getInt*.
> >>>>
> >>>> Should Geode stats be considered a form of API contract and should
> they
> >>>> conform to backwards compatibility constraints?
> >>>>
> >>>> Should we change these from LongCounters back to IntCounters?
> >>>>
> >>>> Someone did suggest that we could change them back to IntCounters and
> >>> then
> >>>> change our statistics internals to skip to zero anytime a stat
> attempts
> >>> to
> >>>> increment above MAX_VALUE, however, I think that if we decide that
> stats
> >>>> cannot be changed from IntCounters to LongCounters then we should not
> >>> make
> >>>> this change either.
> >>>>
> >>>> Thanks for any input or opinions,
> >>>> Kirk
> >>>
> >>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message