You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Darrel Schneider <ds...@pivotal.io> on 2019/06/07 20:32:57 UTC
[DISCUSS] changing geode 32-bit counter stats to 64-bit
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.
Re: [DISCUSS] changing geode 32-bit counter stats to 64-bit
Posted by Dan Smith <ds...@pivotal.io>.
+1 - this sounds like a good change to me.
-Dan
On Fri, Jun 7, 2019 at 1:47 PM Jacob Barrett <jb...@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 <ds...@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.
>
Re: [DISCUSS] changing geode 32-bit counter stats to 64-bit
Posted by Mark Hanson <mh...@pivotal.io>.
+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 <rh...@pivotal.io> wrote:
>
> +1 to @Udo Kohlmeyer <uk...@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 <uk...@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 <ds...@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.
>>
Re: [DISCUSS] changing geode 32-bit counter stats to 64-bit
Posted by Robert Houghton <rh...@pivotal.io>.
+1 to @Udo Kohlmeyer <uk...@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 <uk...@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 <ds...@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.
>
Re: [DISCUSS] changing geode 32-bit counter stats to 64-bit
Posted by Udo Kohlmeyer <uk...@pivotal.io>.
+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 <ds...@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.
Re: [DISCUSS] changing geode 32-bit counter stats to 64-bit
Posted by Jacob Barrett <jb...@pivotal.io>.
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 <ds...@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.