lightning: unlimited grpc MaxCallRecvMsgSize for TikvImporter

[fishiu]

What problem does this PR solve?

Issue Number: close #56114

Problem Summary: Currently, we comply with the default max-message-size=4MiB, but TiKVImporter may receive max 4k KV on each response, and each KV can be about 4KiB+, which exceeds the default limit easily.

What changed and how does it work?

Set grpc options MaxCallRecvMsgSize to math.MaxInt32 (2GiB), i.e., unlimited in most scenarios. This number also aligns with the default setting for MaxCallSendMsgSize in gRPC.

MaxCallRecvMsgSize is only for resource control and there will not be any extra memory consumption. Please refer to the discussions below.

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No need to test

  • I checked and no code files have been changed.

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

[ti-chi-bot]

Hi @fishiu. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tiprow]

Hi @fishiu. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[lance6716]

Can you explain why choose 32 MB, is it enough?

Or can we choose the value after we get the error message like DM?

https://github.com/pingcap/tiflow/blob/76ebb371bb629cc8ca64c04694f985f927634acf/dm/ctl/common/util.go#L131-L139

[fishiu]

Can you explain why choose 32 MB, is it enough?

Or can we choose the value after we get the error message like DM?

https://github.com/pingcap/tiflow/blob/76ebb371bb629cc8ca64c04694f985f927634acf/dm/ctl/common/util.go#L131-L139

32 MB is not a precise number, I chose it only refering to the size mentioned in the issue. Or does @D3Hunter have any idea about the upper bounds in real scenarios?

The way in DM is better. But I first need to locate places that use this client connection and wrap the recv process with adjust-and-retry logics.

[lance6716]

/ok-to-test

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.5678%. Comparing base (aa83e4e) to head (305c863).
Report is 675 commits behind head on master.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #56771         +/-   ##
=================================================
- Coverage   73.1861%   57.5678%   -15.6183%     
=================================================
  Files          1655       1810        +155     
  Lines        456260     657299     +201039     
=================================================
+ Hits         333919     378393      +44474     
- Misses       101837     254123     +152286     
- Partials      20504      24783       +4279     

Flag	Coverage Δ
integration	39.2200% <100.0000%> (?)
unit	73.2973% <0.0000%> (+0.8311%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	52.9478% <ø> (ø)
parser	∅ <ø> (∅)
br	52.4623% <ø> (+6.7526%)	⬆️

[fishiu]

Can you explain why choose 32 MB, is it enough?

Or can we choose the value after we get the error message like DM?

https://github.com/pingcap/tiflow/blob/76ebb371bb629cc8ca64c04694f985f927634acf/dm/ctl/common/util.go#L131-L139

I inspected the grpc source code regarding MaxCallRecvMsgSize and concluded that this option is only a numeric check to limit (memory/bandwidth) resource usage, i.e. setting MaxCallRecvMsgSize to large number will not directly consume large amount of memory. Actually in contrast, MaxCallSendMsgSize is by default maxInt32 in grpc.

Therefore, the strategy to capture the error message and then retry with adjusted options seems to be equivalent with directly setting it to intMax initially.

Additionally, the MaxCallRecvMsgSize is still an experimental option (for many years though) in grpc-go, so currently I do not think it is necessary to add it into lightning's config. Initializing it as maxInt would be simple and clear.

[D3Hunter]

Can you explain why choose 32 MB, is it enough?
Or can we choose the value after we get the error message like DM?
https://github.com/pingcap/tiflow/blob/76ebb371bb629cc8ca64c04694f985f927634acf/dm/ctl/common/util.go#L131-L139

I inspected the grpc source code regarding MaxCallRecvMsgSize and concluded that this option is only a numeric check to limit (memory/bandwidth) resource usage, i.e. setting MaxCallRecvMsgSize to large number will not directly consume large amount of memory. Actually in contrast, MaxCallSendMsgSize is by default maxInt32 in grpc.

Therefore, the strategy to capture the error message and then retry with adjusted options seems to be equivalent with directly setting it to intMax initially.

Additionally, the MaxCallRecvMsgSize is still an experimental option (for many years though) in grpc-go, so currently I do not think it is necessary to add it into lightning's config. Initializing it as maxInt would be simple and clear.

nice insight

[fishiu]

/retest

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamin2037, D3Hunter

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/lightning/OWNERS [Benjamin2037,D3Hunter]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2024-11-05 02:32:02.041780126 +0000 UTC m=+921834.880935671: ☑️ agreed by Benjamin2037.
• 2024-11-06 15:19:24.588605114 +0000 UTC m=+1054277.427760660: ☑️ agreed by D3Hunter.

[ti-chi-bot]

In response to a cherrypick label: new pull request created to branch release-7.1: #57263.

[ti-chi-bot]

In response to a cherrypick label: new pull request created to branch release-8.5: #57346.

[ti-chi-bot]

In response to a cherrypick label: new pull request created to branch release-7.5: #57920.

[ti-chi-bot]

In response to a cherrypick label: new pull request created to branch release-6.5: #59569.