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 <jbarr...@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 <rmcma...@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 <rmcma...@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 <onich...@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 <kl...@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 > >>> > >>> >
