Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

introduce callerID for grpc and apply it for GetRegions #8593

Open
okJiang opened this issue Sep 4, 2024 · 4 comments
Open

introduce callerID for grpc and apply it for GetRegions #8593

okJiang opened this issue Sep 4, 2024 · 4 comments
Labels
type/enhancement The issue or PR belongs to an enhancement.

Comments

@okJiang
Copy link
Member

okJiang commented Sep 4, 2024

Enhancement Task

GetRegion(s) is a hot path and an accident-prone area in PD. In order to better troubleshoot GetRegion(s) in the event of an incident, we decided to record callerID information for it to make it easier to know who was frantically calling GetRegion(s) in the meantime.

@okJiang okJiang added the type/enhancement The issue or PR belongs to an enhancement. label Sep 4, 2024
@okJiang okJiang changed the title introduce callerID for grpc client and apply it for GetRegions introduce callerID for grpc and apply it for GetRegions Sep 4, 2024
@okJiang
Copy link
Member Author

okJiang commented Sep 11, 2024

I would like to put the CallerID and CallerComponent information into the pd header, e.g. okJiang/kvproto@9835bbc. CallerID/CallerComponent can be passed in via ClientOption when the client is created, and should also be allowed to be passed in temporarily each time the interface is called. The priority of ClientOption should be GetRegion(xxx, xxx, ...ClientOption) > CreateClient(xxx, xxx, ...ClientOption).

There is a problem, in the client side, there are very many kinds of options, such as GetStoreOp, RegionsOp, GetRegionOp, due to the client interface in a function can only have one kind of option exists, I tend to unify these Option as ClientOption, all following the rules in the previous paragraph.

What do you think? @rleungx @JmPotato @lhy1024 @nolouch

@JmPotato
Copy link
Member

JmPotato commented Sep 17, 2024

I would like to put the CallerID and CallerComponent information into the pd header, e.g. okJiang/kvproto@9835bbc. CallerID/CallerComponent can be passed in via ClientOption when the client is created, and should also be allowed to be passed in temporarily each time the interface is called. The priority of ClientOption should be GetRegion(xxx, xxx, ...ClientOption) > CreateClient(xxx, xxx, ...ClientOption).

There is a problem, in the client side, there are very many kinds of options, such as GetStoreOp, RegionsOp, GetRegionOp, due to the client interface in a function can only have one kind of option exists, I tend to unify these Option as ClientOption, all following the rules in the previous paragraph.

What do you think? @rleungx @JmPotato @lhy1024 @nolouch

What do you think about this approach to chain calling like the HTTP client? Instead of modifying the function signature, we can return a new client with the CallerID set. We also need to ensure that multiple references to the same client work correctly.

https://github.com/pingcap/tidb/blob/26443dab89ba5b1a866de3c3e44e2f51468bc435/pkg/store/helper/helper.go#L108

@okJiang
Copy link
Member Author

okJiang commented Sep 27, 2024

@JmPotato I think that's a good way to solve this problem as well 👍

@okJiang
Copy link
Member Author

okJiang commented Oct 14, 2024

After sorting

  • TiDB uses the client-go to call GetRegion()
  • TiCDC both uses the client-go and pd-client(client.ScanRegion())
  • BR both uses client-go(RegionCache.LocateKey()) and pd-client(client.GetRegion())
  • Lightning uses pd-client(client.GetRegionByID(), client.ScanRegion())

Since client-go(RegionCache) is a separate repository, the caller need to additionally identify themselves when they create the RegionCache so that the RegionCache knows who it is when it calls pd-client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

No branches or pull requests

2 participants