A new end point to get resource group stats

[cemcayiroglu]

A load balancer can use resource group level stats to do better
load balancing. The new endpoint returns a ResourceGroupInfo.
ResourceGroupInfo contains a list of ResourceGroupInfo's as its subtrees.
ResourceGroupInfo consists of basic stats (running and queued query counts)
and actual queries as strings. This method leaves the queries empty
since it is not needed. The end method does not return the dynamically
generated the resource groups.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ cem cayiroglu (227728a556180e2e0ed5e8a3ac01c728464282a8)

[mayankgarg1990]

A couple of high level questions:

1. Why is the name fullStatsOnly ?
2. The end point returns ResourceGroupInfo that does not contain the running or queued queries -> the resource group object does contain running and queued queries counts - not sure if I understood this line.
3. From an approach of returning all resource group info vs just one, have you thought about having a bulk status import? that way load balancer can cache this state as compared to making on demand calls per incoming query

[cemcayiroglu]

A couple of high level questions:

1. Why is the name fullStatsOnly ?

The end point returns all the stats for a resource group and all it's subtrees. I am open to name it differently.

2. The end point returns ResourceGroupInfo that does not contain the running or queued queries -> the resource group object does contain running and queued queries counts - not sure if I understood this line.

The new endpoint returns a ResourceGroupInfo. ResourceGroupInfo contains a list of ResourceGroupInfo's as its subtrees. ResourceGroupInfo consists of basic stats (running and queued query counts) and actual queries as strings. This method leaves the queries empty since it is not needed.

3. From an approach of returning all resource group info vs just one, have you thought about having a bulk status import? that way load balancer can cache this state as compared to making on demand calls per incoming query

Good question.

The load balancer will call this method with a root group id. It will return everything under that resource group as a ResourceGroupInfo. I think this is the bulk status you are asking.

CC: @mayankgarg1990

[swapsmagic]

Coudn't understand the logic here, how are we deciding if a subgroup is static vs dynamic. can you explain the logic here?

[cemcayiroglu]

resourceGroupSelectionContext is holding the dynamic segment index.
For example:
abc_static.xyx_static.qwe_dynamic.sdq_static.
dynamic segment index is 2.
id.segment.size() is giving the size of the parent. This is also the index of the subgroup. so if parent.segment.size == dynamic segment index than the sub groups is dynamic.

[shixuan-fan]

I don't have much background/context about resource group so my review is very limited :(

How about changing the commit title to: Introduce end point to get static resource group info?

[shixuan-fan]

Why is this true? Sorry I don't have much background about resource group.

[arhimondr]

I guess the root resource group is always static

[cemcayiroglu]

Yes!

[arhimondr]

I'm not an expert in this codebase. But from what I understand the new method should return the resource group information in form of a tree without including the information about running queries and without including the information about dynamically generated subgroups. The extra information about queries and dynamic subgroups is not included to keep the responses short.

Currently there's already a method that returns resource group information that includes detailed query information and a high level summary for the subgroups.

I wonder if these two methods should be merge together by introducing additional parameters. I don't have a strong opinion about this, and would love to hear what others think.

[arhimondr]

I guess the root resource group is always static

[swapsmagic]

i think this logic works to identify the given resource group is static or not. But a much cleaner way would be, if we mark each segment of ResourceGroupId being static or not when we create them via group.expandTemplate call, then we can easily put the flag here. And no need to rely on the context.

i.e. this line will become something like this
group = parent.getOrCreateSubGroup(id.getLastSegment().getValue(), id.getLastSegment().isStatic());

And i think to make this happen, we may need to change the segments in ResourceGroupId class from string to a class Segment which looks like this:
class Segment {
String value;
String isStatic;
}

[cemcayiroglu]

We had a offline chat with @swapsmagic. We concluded that introducing a segment class would require a major code change. We are going to keep it as is.

[arhimondr]

Looks good

[swapsmagic]

few naming related comment, rest looks good.

[swapsmagic]

i think this should have been named as staticSubGroup

[swapsmagic]

summarizeSubgroups should be summarizeSubGroups to make it true camel case.