add resource group name in request context

[glorv]

No description provided.

[glorv]

@sticnarf @Connor1996 PTAL

[sticnarf]

Must we use the name "resource group"? Two unrelated concepts sharing the same name is really confusing... (Though I think it's more of the original resourceGroupTag's fault. The name "resource group" is too large for that)

[glorv]

The change in pingcap/kvproto#1029 used ResourceGroupName. I think it's somewhat confusing between the resource group tag and the new resource group.

[Connor1996]

Indeed. How about changing the original name resourceGroupTag? Though I have no idea what to change to. /cc @mornyx

[mornyx]

The change in pingcap/kvproto#1029 used ResourceGroupName. I think it's somewhat confusing between the resource group tag and the new resource group.

Could you please briefly explain what resource_group_name does?

The previous resource_group_tag was used with tikv#resource_metering module. They work together to measure the resource (CPU/Memory/...) usage of different SQLs. But we try to avoid introducing the concept of "SQL" to TiKV, so we only use a more general name.

I agree that maybe resource_group_tag needs a better name, but I need to make this decision based on both of our backgrounds.

(If only to avoid confusion, maybe resource_metering_tag is an option, which at least avoids the common resource_group_ prefix.)

[Connor1996]

The resource group is a term used for resource control of different tenants or users. Each resource group defines resource quotas that it can use, like the number of CPU cores, read bandwidth and write bandwidth.

[Connor1996]

Yes, I think resource_metering_tag is a more reasonable name.

[BornChanger]

The change in pingcap/kvproto#1029 used ResourceGroupName. I think it's somewhat confusing between the resource group tag and the new resource group.

Could you please briefly explain what resource_group_name does?

The previous resource_group_tag was used with tikv#resource_metering module. They work together to measure the resource (CPU/Memory/...) usage of different SQLs. But we try to avoid introducing the concept of "SQL" to TiKV, so we only use a more general name.

I agree that maybe resource_group_tag needs a better name, but I need to make this decision based on both of our backgrounds.

(If only to avoid confusion, maybe resource_metering_tag is an option, which at least avoids the common resource_group_ prefix.)

What I want to make sure that the renaming is only seen internally. No external user interface will be affected, including metrics indicators. Is that true?

[mornyx]

What I want to make sure that the renaming is only seen internally. No external user interface will be affected, including metrics indicators. Is that true?

We need to update TiDB/TiKV/kvproto/client-go at the same time. I think kvproto and client-go are the public interfaces of TiKV. If users build their own database system based on TiKV and depend on this field, then there is a breaking change here (although I think this is impossible, because this field is currently only used for our TopSQL). Other than this, no external user interface is affected.

/cc @zhongzc @breezewish

[breezewish]

I think it's fine, because renaming a protobuf field name should be a compatible change. @mornyx

[glorv]

So shall I conclude that we will keep the current definition unchanged. @sticnarf @nolouch PTAL again

[nolouch]

lgtm. I support to change resource_group_tag to resource_metering_tag or metering_tag

[BornChanger]

lgtm

[Connor1996]

LGTM

[glorv]

@sticnarf PTAL again, thanks

[sticnarf]

I find the lock resolver sends requests without the resource group name. Typically, the amount should be small. Are you going to set resource group name also for lock resolving requests?

[glorv]

I find the lock resolver sends requests without the resource group name. Typically, the amount should be small. Are you going to set resource group name also for lock resolving requests?

@sticnarf. Sorry I missed it. I fixed this by fetch the resource group name from the context, so the caller can bind this kind of requests to a specific resource group.
PTAL again, thanks~