Thank you for your feedback, Owen. This change has been merged to develop. On Sat, Feb 23, 2019 at 7:26 AM Owen Nichols <onich...@pivotal.io> wrote:
> This seems like an entirely reasonable bugfix to make in a minor release. > A “release note” mentioning the change should be more than sufficient to > alert anyone who may be directly accessing stats. > > For me, the key information here is "This change does not affect VSD”. > For most people, VSD is the "public interface" to statistics. > > +1 for moving from ints to longs wherever possible > > -Owen > > > On Feb 22, 2019, at 11:00 AM, Helena Bales <heyba...@apache.org> wrote: > > > > Hello all, > > > > The performance team wants to change some of the statistics that are > > currently ints to be longs so that they do not overflow as quickly. We > were > > encountering issues with the performance tests due to overflows in stats > > following an increase in performance for gets. Recent pull requests to > make > > this change have been reverted due to concern that this changes "public > > APIs". In our current proposed change we did not change the types of the > > MBean stats, however the stats are technically accessible from the > > distributed system. They can be accessed as long as you know the name of > > that statistic, but the names are not all correctly documented. Accessing > > internals through incorrectly documented identifiers does not constitute > a > > public API but since it is technically something a user could do, this > > change demands some discussion. We have done our best to mitigate any > > potential issues for the user by downcasting the long stats to ints when > > the stats are retrieved by name only. If the user knew the unique id of > > that stat, and then tried to retrieve it as an int, they could > potentially > > retrieve the wrong counter, or an exception if that id was not valid for > > that type. This is because the unique id is unique within that type of > > stat, not unique for all stats (see the second link below). Updating an > > internal stat by name or id will also fail for those stats we have > changed > > but it is strongly arguable that updating internal stats should not be > done > > externally by users. This change does not affect VSD or MBeans. > > > > Does anyone believe that there is a valid use case for accessing internal > > statistics not addressed by this change? > > > > Thanks, > > Helena and Jake > > > > The PR: https://github.com/apache/geode/pull/3214 > > The test that shows our workaround: > > > https://github.com/apache/geode/pull/3214/files#diff-42320800db41504616849361b4b2a3af > >
