+1 to @Udo Kohlmeyer <ukohlme...@pivotal.io> comment. If the memory has been used, might as well allow us to query the values
On Fri, Jun 7, 2019 at 2:18 PM Udo Kohlmeyer <ukohlme...@pivotal.io> wrote: > +1, if the LongAdders have already added and the additional memory usage > has already been dealt with, then adding the accessors does not really > make a difference anymore. > > On 6/7/19 13:47, Jacob Barrett 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. >
