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
