+1 to what Jake said about MemberMXBean - that is definitely a public API class, so we need to deprecate the old method and introduce a new one.
I'm also not clear about the stats. Technically, they are discoverable through the public API, so maybe? It seems like they are mix of things users might want and stuff that is more internal/debugging oriented. If the downcasting trick Jake mentioned isn't that hard, I'd say we should do that. That said, I suspect we've renamed some stats in the past. One other thing to consider - will changing the size of the stats affect any tooling like VSD or gfsh or ?? that may have to deal with stat data from multiple geode versions? -Dan On Wed, Feb 13, 2019 at 3:59 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 >
