I accept that point. I would certainly like to minimize the work and per bucket status would seem undesirable. I can totally agree to back burner that until some point in the future at which point we agree to move forward on it. I was just anticipating that would fall out necessarily. That said, let’s consider that back burnered for this proposal.
The main classes I really want to cleanup with this proposal are CachePerfStats and RegionPerfStats. Thanks for the good constructive feedback. > On Jul 11, 2019, at 9:36 AM, Jacob Barrett <jbarr...@pivotal.io> wrote: > > So is the root of this proposal really about expanding our current stats > collection to include per-bucket stats. If not I would really separate these > two ideas. First focus on refactoring these stats classes to be more > manageable and readable. Then propose adding per-bucket stats as a thing > separately. If this discussion is really about per-bucket stats then let’s > focus the subject on that and not really worry about any internal refactoring > that must happen to make it work. > >> On 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 >>>>>>> >>>>>> >>>>>> >>>> >> >
