-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
load data: physical mode part1 #42817
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
executor/importer/chunk_process.go
Outdated
|
||
checksum verify.KVChecksum | ||
encoder kvEncoder | ||
kvStore tidbkv.Storage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use codec instead of kvstore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lightning uses it to alloc auto-id in common.AllocGlobalAutoID
too, but this place can be removed
/test all |
/retest |
tableMeta := &mydump.MDTableMeta{ | ||
DB: e.DBName, | ||
Name: e.Table.Meta().Name.O, | ||
DataFiles: e.toMyDumpFiles(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add TODO for IsRowOrdered
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 / 11 files viewed
executor/importer/chunk_process.go
Outdated
|
||
func (b *deliverKVBatch) add(kvs *kv.Pairs) { | ||
for _, pair := range kvs.Pairs { | ||
if pair.Key[tablecodec.TableSplitKeyLen+1] == 'r' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use tablecodec.IsRecordKey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deliverCompleteCh := make(chan deliverResult) | ||
go func() { | ||
defer close(deliverCompleteCh) | ||
err := p.deliverLoop(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not important, but I prefer the error group pattern. When the job is finished, close the channel to notify the consumer. When any loop meets error, error group will automatically cancel the derived context and the error can be found by eg.Wait()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplified from lightning code, will add a comment and do it later
/retest |
/retest |
multiple-valued index related test will add later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest lgtm
s.tk.MustExec("use " + db) | ||
} | ||
|
||
func (s *mockGCSSuite) TestPhysicalMode() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In future we can move some tests to a common package, and let different backend share the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
physical related can only run in realtikvtest
(check_dev_2
), we can move logical test here.
tidbCfg := tidb.GetGlobalConfig() | ||
// todo: add job id too | ||
sortPathSuffix := "import-" + strconv.Itoa(int(tidbCfg.Port)) | ||
sortPath := filepath.Join(tidbCfg.TempDir, sortPathSuffix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found both TempDir and TempStoragePath and they don't have comment, not sure how to use them. @tangenta can you help us?
And will lightning's builtin disk quota conflicts with TiDB's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copied from here
Line 88 in 7c2c992
sortPath := filepath.Join(tidbCfg.TempDir, sortPathSuffix) |
will lightning's builtin disk quota conflicts with TiDB
yes, we should precheck whether's ongoing add-index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No a big problem, we can check it later @tangenta PTAL at your convenience
ConnCompressType: config.CompressionNone, | ||
WorkerConcurrency: config.DefaultRangeConcurrency * 2, | ||
KVWriteBatchSize: config.KVWriteBatchSize, | ||
CheckpointEnabled: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to turn on checkpoint now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local backend report error when the sort-dir already exists & checkpoint disabled
will add a todo and fix it later
executor/importer/table_import.go
Outdated
DupeDetectEnabled: false, | ||
DuplicateDetectOpt: local.DupDetectOpt{ReportErrOnDup: false}, | ||
StoreWriteBWLimit: 0, | ||
ShouldCheckWriteStall: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be true because we don't periodically switch import mode so the cluster may be slow to ingest. Today I meet a ONCALL 😭
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 66d0c78
|
/retest |
What problem does this PR solve?
Issue Number: ref #40499
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.