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