+1 - this sounds like a good change to me. -Dan
On Fri, Jun 7, 2019 at 1:47 PM Jacob Barrett <jbarr...@pivotal.io> wrote: > I like this! > > I’d go ahead and change all the usage of the int methods to the long > methods. I’d deprecate the int methods to make it very clear. > > If some consumer is using the int methods they will still work with the > same rollover issues but perhaps with the deprecated warning they will > update their code. Without the warning they may never know. > > -jake > > > > On Jun 7, 2019, at 1:32 PM, Darrel Schneider <dschnei...@pivotal.io> > wrote: > > > > We have had a couple of tickets that have problems with 32-bit counters > > changing too fast and causing them to be hard to understand when they > wrap > > around (see GEODE-6425 and GEODE-6834). We have also had problems with > some > > stats being broken because they were changing the 32-bit one when they > > should have been changing the 64-bit one (see GEODE-6776). > > The current implementation has one array of values for the 32-bit stats > and > > another array of values for the 64-bit stats. We use indexes into these > > arrays when changing a stat. Given an int "id" used to index these > arrays, > > we can not tell if we should be indexing the 32-bit array or 64-bit > array. > > The first 32-bit stat for a type will have an id of 0 and the first > 64-bit > > stat on that type will also have an id of 0. But our current > implementation > > has the same type of value in both arrays (LongAdder > > see: StripedStatisticsImpl fields intAdders and longAdders). So if we > > simply change our implementation to have a single array, then the ids > will > > no longer be overloaded. > > > > Changing this internal implementation also improves backward > compatibility. > > Currently when we change one of our counters from 32-bit to 64-bit it is > > possible we would break existing applications that are using the > Statistics > > interface. It has methods on it that allow stats to be read given an it: > > getInt(int id) and getLong(int id). If they are currently reading a > 32-bit > > stat using getInt(id) and we change that stat to be 64-bit (like we did > in > > GEODE-6425) then they will now be reading the wrong stat when they call > > getInt(id). If they used getInt(String) or getInt(StatisticDescriptor) > they > > will be okay. But if we make this simple change to have 32-bit and 64-bit > > stats stored in the same array then getInt(id) will do the right thing > when > > we change a stat from 32-bit to 64-bit. > > > > Does anyone see a problem with making this change? > > > > After we do it I think we should change all of our counters to 64-bit > since > > they are always stored in a LongAdder anyway and we should deprecate the > > methods that support 32-bit stats. >
