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

Remove getRegions call by lightning in physical import mode #45507

Closed
mittalrishabh opened this issue Jul 21, 2023 · 7 comments · Fixed by #46202
Closed

Remove getRegions call by lightning in physical import mode #45507

mittalrishabh opened this issue Jul 21, 2023 · 7 comments · Fixed by #46202
Assignees
Labels
affects-5.3 This bug affects 5.3.x versions. affects-5.4 This bug affects 5.4.x versions. affects-6.1 affects-6.5 affects-7.1 component/lightning This issue is related to Lightning of TiDB. severity/major type/bug The issue is confirmed as a bug.

Comments

@mittalrishabh
Copy link
Contributor

mittalrishabh commented Jul 21, 2023

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

When lightning runs in physical import mode, it runs this sql query

SELECT REGION_ID, APPROXIMATE_SIZE FROM information_schema.TIKV_REGION_STATUS WHERE TABLE_ID = ?

This call is made by each lightning process in parallel import mode and by multiple worker threads in a lightning process. It can potentially overload the PD if number of regions are too many. However, it appears that this call may be unnecessary since its purpose is to display a warning message and increment a counter in the SplitAndScatterRegionByRanges() function. Considering that lightning physical import always operates on a new table, splitting the new region should not pose any harm.

2. What did you expect to see? (Required)

Less number of get region calls to PD

3. What did you see instead (Required)

4. What is your TiDB version? (Required)

6.5

@mittalrishabh mittalrishabh added the type/bug The issue is confirmed as a bug. label Jul 21, 2023
@mittalrishabh mittalrishabh changed the title Lightning calling getRegion in physical import mode Remove getRegions by lightning in physical import mode Jul 21, 2023
@mittalrishabh mittalrishabh changed the title Remove getRegions by lightning in physical import mode Remove getRegions call by lightning in physical import mode Jul 21, 2023
@AndreMouche
Copy link
Contributor

It is a bug in this version since we have #39605 .
Meanwhile,As the @mittalrishabh mentioned, is it necessary to take the region of the new table here? Is there any room for optimization?

@AndreMouche
Copy link
Contributor

@niubell Please take a look, thanks

@lance6716
Copy link
Contributor

Yes, seems the correct behaviour is continue the loop for a skippedKey

sendLoop:
for regionID, keys := range splitKeyMap {
// if region not in tableRegionStats, that means this region is newly split, so
// we can skip split it again.
regionSize, ok := tableRegionStats[regionID]
if !ok {
log.FromContext(ctx).Warn("region stats not found", zap.Uint64("region", regionID))
}
if len(keys) == 1 && regionSize < regionSplitSize {
skippedKeys++
}
select {
case ch <- &splitInfo{region: regionMap[regionID], keys: keys}:
case <-ctx.Done():
// outer context is canceled, can directly return
close(ch)
return ctx.Err()
case <-splitCtx.Done():
// met critical error, stop process
break sendLoop
}
}

But since we don't have a continue in code and everything works fine, I think remove the whole checking logic is acceptable. PTAL @niubell

@seiya-annie seiya-annie added severity/major component/lightning This issue is related to Lightning of TiDB. labels Jul 24, 2023
@ti-chi-bot ti-chi-bot bot added may-affects-5.2 This bug maybe affects 5.2.x versions. may-affects-5.3 This bug maybe affects 5.3.x versions. may-affects-5.4 This bug maybe affects 5.4.x versions. may-affects-6.1 may-affects-6.5 may-affects-7.1 labels Jul 24, 2023
@kennytm
Copy link
Contributor

kennytm commented Aug 15, 2023

hi @niubell any update on this issue? thanks

@mittalrishabh
Copy link
Contributor Author

I am going to create a PR for this issue. I have tested it internally.

@lance6716
Copy link
Contributor

@mittalrishabh thanks!

@mittalrishabh
Copy link
Contributor Author

filed PR #46202

@lance6716 lance6716 added affects-5.3 This bug affects 5.3.x versions. affects-5.4 This bug affects 5.4.x versions. affects-6.1 affects-6.5 and removed may-affects-5.2 This bug maybe affects 5.2.x versions. may-affects-5.3 This bug maybe affects 5.3.x versions. may-affects-5.4 This bug maybe affects 5.4.x versions. may-affects-6.1 may-affects-6.5 may-affects-7.1 labels Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-5.3 This bug affects 5.3.x versions. affects-5.4 This bug affects 5.4.x versions. affects-6.1 affects-6.5 affects-7.1 component/lightning This issue is related to Lightning of TiDB. severity/major type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants