+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 > >
