[GitHub] [geode] kirklund commented on issue #3142: GEODE-6334: Change cache operation stats to longs

["kirklund (GitHub)" <gi...@apache.org>]

I'm not sure if there are stats that have legitimately negative values or not. I can't think of any but there might be some. 

If we change the underlying stats framework to not allow negative values, we have two possible approaches: 1) have the code throw if a call attempts to drop the value below zero, 2) have the code ignore and decrements when the value is at zero.

For #1, I'm worried that we would create new failures that would bring down servers when the stats used to go negative.

For #2, I'm a little worried that we would suppress our ability to notice that code decrementing a stat is buggy. I don't think we want to log every time we try to dec below zero to avoid suppressing this.

A bigger problem that Michael and I noticed is that there's no type enforcement. In other words if the stat is defined as a LongCounter but some code incs or decs it as an Integer, nothing complains or fails. The ThreadLocal used for striping values has two arrays: one for long values and one for integer values. No matter which Counter type, incInt or incLong will happily update the value in corresponding array. So if a stat has some bug in which most of the code paths invoke incLong but one path still invokes incInt, it'll fail to update the proper underlying array and the call to incInt is affectively ignored for the stat. The reason this isn't easy to fix is that there's a single class (Atomic50StatisticsImpl) for all stats in a given *Stats class which may contain a mix of both long and integer stats. I think fixing this problem would be a lot of work redoing the stats framework.

I wonder if it would be better to start migrating ALL stats implementations to using Micrometer instead of reworking our existing framework. (both for dec'ing stats below zero and long vs integer type safety)

[ Full content available at: https://github.com/apache/geode/pull/3142 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org