+1 for Jake’s approach. Jake’s approach seems to address the only concern that I know of which is backward compatibility. Thanks, Mark
> On Jun 10, 2019, at 11:20 AM, Robert Houghton <rhough...@pivotal.io> wrote: > > +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. >>
