You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Mark Hanson <mh...@pivotal.io> on 2019/07/10 23:49:20 UTC
[Proposal] Refactor the Cache and Region perf stats structure.
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
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Kirk Lund <kl...@apache.org>.
I would recommend doing a spike for this refactoring before trying to
provide a class diagram -- I would personally learn a lot that would have a
major effect on the resulting class design that I can only guess about up
front.
Another major challenge is in trying to keep the resulting .gfs file
unchanged while straightening up the class design. The effort may be
worthwhile, but I don't think the spike will be quick or trivial.
The main reason to have any 1-to-many relationships in this class design
(ex: CacheStats to RegionStats) is to build in aggregation, such as number
of puts per Region and number of puts across every Region in the Cache. We
also need PartitionedRegion aggregates such as number of puts across every
BucketRegion for a specific PartitionedRegion -- which is also a 1-to-many
of PartitionedRegionStats to (Bucket)RegionStats relationship (and of
course a Cache can have many PartitionedRegions). I would prefer to come up
with some sort of reusable statistic aggregation design as part of the
spike.
On Wed, Jul 10, 2019 at 4:49 PM Mark Hanson <mh...@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
>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Mark Hanson <mh...@pivotal.io>.
My personal thinking was:
Step 1: fix the object structure this round, and try to keep the data output
the same. The downside here is that a chunk of stuff from CachePerfStats would then be duplicated into
RegionPerfStats directly where it was there effectively. This also means that PartitionedRegionStats stops being a standalone class.
It becomes an Implementation of RegionStats (note the name).
Step 2: Add in Metrics facade to track the existing stats.
Step 3: Move existing stats calls to use Metrics decorator.
Step 3: Optional, but desirable, remove unnecessary metrics from RegionStats (note the name change), that really are CacheStats. This would break backward compatibility.
Thanks,
Mark
> On Jul 11, 2019, at 10:33 AM, Darrel Schneider <ds...@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 <mh...@pivotal.io> wrote:
>
>> See my comments inline..
>>
>>> On Jul 11, 2019, at 9:36 AM, Darrel Schneider <ds...@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 <mh...@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 <jb...@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 <mh...@pivotal.io> wrote:
>>>>>>
>>>>>> Correct.
>>>>>>
>>>>>>> On Jul 11, 2019, at 9:23 AM, Darrel Schneider <dschneider@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 <mh...@pivotal.io>
>>>> wrote:
>>>>>>>>
>>>>>>>> PartitionRegionStatsImpl can contain one to many RegionStats
>>>>>>>>
>>>>>>>>> On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io
>>>> <mailto:
>>>>>>>> mhanson@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
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Mark Hanson <mh...@pivotal.io>.
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 <ds...@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 <mh...@pivotal.io> wrote:
>
>> See my comments inline..
>>
>>> On Jul 11, 2019, at 9:36 AM, Darrel Schneider <ds...@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 <mh...@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 <jb...@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 <mh...@pivotal.io> wrote:
>>>>>>
>>>>>> Correct.
>>>>>>
>>>>>>> On Jul 11, 2019, at 9:23 AM, Darrel Schneider <dschneider@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 <mh...@pivotal.io>
>>>> wrote:
>>>>>>>>
>>>>>>>> PartitionRegionStatsImpl can contain one to many RegionStats
>>>>>>>>
>>>>>>>>> On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io
>>>> <mailto:
>>>>>>>> mhanson@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
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Darrel Schneider <ds...@pivotal.io>.
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 <mh...@pivotal.io> wrote:
> See my comments inline..
>
> > On Jul 11, 2019, at 9:36 AM, Darrel Schneider <ds...@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 <mh...@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 <jb...@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 <mh...@pivotal.io> wrote:
> >>>>
> >>>> Correct.
> >>>>
> >>>>> On Jul 11, 2019, at 9:23 AM, Darrel Schneider <dschneider@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 <mh...@pivotal.io>
> >> wrote:
> >>>>>>
> >>>>>> PartitionRegionStatsImpl can contain one to many RegionStats
> >>>>>>
> >>>>>>> On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io
> >> <mailto:
> >>>>>> mhanson@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
> >>>>>>>
> >>>>>>
> >>>>>>
> >>>>
> >>
> >>
>
>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Mark Hanson <mh...@pivotal.io>.
See my comments inline..
> On Jul 11, 2019, at 9:36 AM, Darrel Schneider <ds...@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 <mh...@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 <jb...@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 <mh...@pivotal.io> wrote:
>>>>
>>>> Correct.
>>>>
>>>>> On Jul 11, 2019, at 9:23 AM, Darrel Schneider <ds...@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 <mh...@pivotal.io>
>> wrote:
>>>>>>
>>>>>> PartitionRegionStatsImpl can contain one to many RegionStats
>>>>>>
>>>>>>> On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io
>> <mailto:
>>>>>> mhanson@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
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>
>>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Darrel Schneider <ds...@pivotal.io>.
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?
Currently we have an internal CachePerfStats which the internal
RegionPerfStats extends. Does your CacheStats replace CachePerfStats and
your RegionStats replace RegionPerfStats?
Currently we have an internal PartitionedRegionStats class. Does
your PartitionedRegionStats replace it?
Are your "*Stats" interfaces and your "*StatsImpl" classes?
On Thu, Jul 11, 2019 at 9:29 AM Mark Hanson <mh...@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 <jb...@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 <mh...@pivotal.io> wrote:
> >>
> >> Correct.
> >>
> >>> On Jul 11, 2019, at 9:23 AM, Darrel Schneider <ds...@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 <mh...@pivotal.io>
> wrote:
> >>>>
> >>>> PartitionRegionStatsImpl can contain one to many RegionStats
> >>>>
> >>>>> On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io
> <mailto:
> >>>> mhanson@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
> >>>>>
> >>>>
> >>>>
> >>
>
>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Mark Hanson <mh...@pivotal.io>.
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 <jb...@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 <mh...@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 <jb...@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 <mh...@pivotal.io> wrote:
>>>>
>>>> Correct.
>>>>
>>>>> On Jul 11, 2019, at 9:23 AM, Darrel Schneider <ds...@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 <mh...@pivotal.io> wrote:
>>>>>>
>>>>>> PartitionRegionStatsImpl can contain one to many RegionStats
>>>>>>
>>>>>>> On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io <mailto:
>>>>>> mhanson@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
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>
>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Jacob Barrett <jb...@pivotal.io>.
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 <mh...@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 <jb...@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 <mh...@pivotal.io> wrote:
>>>
>>> Correct.
>>>
>>>> On Jul 11, 2019, at 9:23 AM, Darrel Schneider <ds...@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 <mh...@pivotal.io> wrote:
>>>>>
>>>>> PartitionRegionStatsImpl can contain one to many RegionStats
>>>>>
>>>>>> On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io <mailto:
>>>>> mhanson@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
>>>>>>
>>>>>
>>>>>
>>>
>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Mark Hanson <mh...@pivotal.io>.
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 <jb...@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 <mh...@pivotal.io> wrote:
>>
>> Correct.
>>
>>> On Jul 11, 2019, at 9:23 AM, Darrel Schneider <ds...@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 <mh...@pivotal.io> wrote:
>>>>
>>>> PartitionRegionStatsImpl can contain one to many RegionStats
>>>>
>>>>> On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io <mailto:
>>>> mhanson@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
>>>>>
>>>>
>>>>
>>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Jacob Barrett <jb...@pivotal.io>.
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 <mh...@pivotal.io> wrote:
>
> Correct.
>
>> On Jul 11, 2019, at 9:23 AM, Darrel Schneider <ds...@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 <mh...@pivotal.io> wrote:
>>>
>>> PartitionRegionStatsImpl can contain one to many RegionStats
>>>
>>>> On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io <mailto:
>>> mhanson@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
>>>>
>>>
>>>
>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Mark Hanson <mh...@pivotal.io>.
Correct.
> On Jul 11, 2019, at 9:23 AM, Darrel Schneider <ds...@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 <mh...@pivotal.io> wrote:
>
>> PartitionRegionStatsImpl can contain one to many RegionStats
>>
>>> On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io <mailto:
>> mhanson@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
>>>
>>
>>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Darrel Schneider <ds...@pivotal.io>.
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 <mh...@pivotal.io> wrote:
> PartitionRegionStatsImpl can contain one to many RegionStats
>
> > On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io <mailto:
> mhanson@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
> >
>
>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Mark Hanson <mh...@pivotal.io>.
PartitionRegionStatsImpl can contain one to many RegionStats
> On Jul 10, 2019, at 4:53 PM, Dan Smith <ds...@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 <mhanson@pivotal.io <ma...@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
>
Re: [Proposal] Refactor the Cache and Region perf stats structure.
Posted by Dan Smith <ds...@pivotal.io>.
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 <mh...@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
>