geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Hanson <mhan...@pivotal.io>
Subject Re: [DISCUSS] changing geode 32-bit counter stats to 64-bit
Date Mon, 10 Jun 2019 22:00:23 GMT
+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 <rhoughton@pivotal.io> wrote:
> 
> +1 to @Udo Kohlmeyer <ukohlmeyer@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 <ukohlmeyer@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 <dschneider@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.
>> 


Mime
View raw message