Hello All, Since there is no one disagreeing, we are going to start moving forward with this.
Thanks, Mark > On Jul 11, 2019, at 10:33 AM, Darrel Schneider <dschnei...@pivotal.io> wrote: > > Originally we just had a single instance of CachePerfStats per jvm. So all > the region related stats were always rolled up onto the single > CachePerfStats. Later we added RegionPerfStats so that users could see what > was happening on a per region basis. When RegionPerfStats was added it was > made to extend CachePerfStats but no new stats were added to it. > > I think we now have some stats that only make sense on a Cache (like > "regions" and "partitionedRegions") but we also have them on > RegionPerfStats. I thought these used to always return zero on the region > but I just looked at the implementation and it looks like they just return > 1 or 0 on the region (depending on if it is partitioned or not). The help > text says it will be the number of regions on the cache (so the help is > correct for CachePerfStats but wrong for RegionPerfStats). I would be okay > with us dropping the stats that only make sense at the cache level from the > per region stats. > > Something I could not tell from you diagram is if you are cleaning this up. > Does CacheStats also have everything that is on RegionStats? Or will it now > just have the stats that are unique to a cache? > > If you change the internal names (like CachePerfStats -> CacheStats) keep > in mind that you should use the same type name when calling "createType" > (in this case "CachePerfStats") for backwards compatibility. > > On Thu, Jul 11, 2019 at 9:45 AM Mark Hanson <mhan...@pivotal.io> wrote: > >> See my comments inline.. >> >>> On Jul 11, 2019, at 9:36 AM, Darrel Schneider <dschnei...@pivotal.io> >> wrote: >>> >>> Currently we do not make visible per bucket stats. Is the above proposal >>> just to change the internal implementation but the stats visible in tools >>> like vsd are unchanged? >>> >> As with my previous e-mail exchange with Jake, I would like to back burner >> per bucket stats. If it turns out to be a problem, I will bring it back >> before the group. >> >> My goal is these are internal changes. I would see it as a problem >> requiring re-evaluation, if this were a meaningful breaking change. E.g. >> breaks vsd, changes public API >> An important note would be that fixing a bug in a stat, is not a >> meaningful breaking change. >> >>> Currently we have an internal CachePerfStats which the internal >>> RegionPerfStats extends. Does your CacheStats replace CachePerfStats and >>> your RegionStats replace RegionPerfStats? >>> >> >> You are 100% correct. >> >>> Currently we have an internal PartitionedRegionStats class. Does >>> your PartitionedRegionStats replace it? >>> >> >> Yes. I considered that under the “RegionPerfStats” umbrella. >> >>> Are your "*Stats" interfaces and your "*StatsImpl" classes? >> >> Yes. >> >>> >>> On Thu, Jul 11, 2019 at 9:29 AM Mark Hanson <mhan...@pivotal.io> wrote: >>> >>>> It depends to be honest. This is just a plan. I would hope to roll them >>>> up, but I can see a case where you have buckets on different servers, >> that >>>> would seem to necessitate that. >>>> >>>>> On Jul 11, 2019, at 9:26 AM, Jacob Barrett <jbarr...@pivotal.io> >> wrote: >>>>> >>>>> There isn’t currently a partition stat instance per bucket. Are you >>>> saying you’re making that a thing now? >>>>> >>>>>> On Jul 11, 2019, at 9:24 AM, Mark Hanson <mhan...@pivotal.io> wrote: >>>>>> >>>>>> Correct. >>>>>> >>>>>>> On Jul 11, 2019, at 9:23 AM, Darrel Schneider <dschnei...@pivotal.io >>> >>>> wrote: >>>>>>> >>>>>>> Why would a PartitionedRegionStatsImpl contain more than one >>>> RegionStats? >>>>>>> Are these representing the local buckets? >>>>>>> >>>>>>>> On Wed, Jul 10, 2019 at 4:57 PM Mark Hanson <mhan...@pivotal.io> >>>> wrote: >>>>>>>> >>>>>>>> PartitionRegionStatsImpl can contain one to many RegionStats >>>>>>>> >>>>>>>>> On Jul 10, 2019, at 4:53 PM, Dan Smith <dsm...@pivotal.io> wrote: >>>>>>>>> >>>>>>>>> Seems reasonable. I'm guessing that CachePerfImpl contains many >>>>>>>> RegionStats. Does PartitionRegionStatsImpl just contain a single >>>>>>>> RegionStats? >>>>>>>>> >>>>>>>>> On Wed, Jul 10, 2019 at 4:49 PM Mark Hanson <mhan...@pivotal.io >>>> <mailto: >>>>>>>> mhan...@pivotal.io>> wrote: >>>>>>>>> Hi All, >>>>>>>>> >>>>>>>>> As many of you may know our structure for our perf stats is not >>>> great. I >>>>>>>> would like to propose we refactor the code to have the following >>>>>>>> inheritance model, which Kirk and I came up with. >>>>>>>>> >>>>>>>>> It is my belief that fixing this will allow future features to be >>>>>>>> implemented in a much less painful way. >>>>>>>>> >>>>>>>>> Thoughts? >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Mark >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>> >>>> >>>> >> >>
