Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

restore: add importing progress and optimize the accuracy of restore progress #506

Merged
merged 9 commits into from
Jan 27, 2021

Conversation

glorv
Copy link
Contributor

@glorv glorv commented Dec 3, 2020

What problem does this PR solve?

Better progress calculation and log
resolves partially #440

What is changed and how it works?

  • Remove the progress of file and add progress of engine(count for import). After all files restore finished, the state will be 'importing' instead of 'post-progress'. And after all engines import finished, the state will be 'post-progress'.
  • Add engine count metrics at estimating chunks count
  • Use the total pending chunks count instead of estimated chunks if pending chunk count > estimated chunk count to gain better accuracy. This can avoid the chunks progress exceeding 100%
  • Add total progress percentage. The value of total progress is estimatedWriteProgress * 0.8 + estimatedImportProgress * 0.2
  • Calculate chunk count based on chunk start offset to be consistent with the chunk count estimation logic.
  • Flush data and index engine and save all chunk checkpoints so that if restore engine meets context cancel all chunk checkpoints are saved.

New log output style:

[2020/12/07 00:59:44.725 +08:00] [INFO] [restore.go:694] [progress] [total=57.2%] [tables="7/9 (77.8%)"] [chunks="2642/4247 (62.2%)"] [engines="17/27 (63.0%)"] [speed(MiB/s)=131.06370869464234] [state=writing] [remaining=52m21s]
[2020/12/07 01:09:44.725 +08:00] [INFO] [restore.go:694] [progress] [total=65.0%] [tables="7/9 (77.8%)"] [chunks="3020/4247 (71.1%)"] [engines="18/27 (66.7%)"] [speed(MiB/s)=134.3286817015894] [state=writing] [remaining=43m10s]
[2020/12/07 01:59:44.725 +08:00] [INFO] [restore.go:694] [progress] [total=97.4%] [tables="7/9 (77.8%)"] [chunks="4601/4601 (100.0%)"] [engines="25/27 (92.6%)"] [speed(MiB/s)=132.1332473121803] [state=importing] [remaining=3m30s]

Check List

Tests

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

Side effects

Related changes

@july2993
Copy link
Contributor

count you also add the estimated progress in /progress/task and /progress/table

@glorv
Copy link
Contributor Author

glorv commented Dec 14, 2020

count you also add the estimated progress in /progress/task and /progress/table

Lightning doesn't support return the progress for any individual table. Please submit a feature request issue.

@@ -486,6 +487,11 @@ func (local *local) getImportClient(ctx context.Context, peer *metapb.Peer) (sst
return sst.NewImportSSTClient(conn), nil
}

type rangeStats struct {
count int64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems this member is not used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, currently only uses the totalBytes

lightning/restore/restore.go Show resolved Hide resolved
lightning/restore/restore.go Outdated Show resolved Hide resolved
}
metric.ChunkCounter.WithLabelValues(metric.ChunkStatePending).Inc()
remainChunkCnt := float64(chunk.Chunk.EndOffset-chunk.Chunk.Offset) / float64(chunk.Chunk.EndOffset-chunk.Key.Offset)
Copy link
Contributor

@lance6716 lance6716 Jan 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the meaning of chunk.Key.Offset, when does it not equal chunk.Chunk.Offset. 🤔

And hope there is no division by zero error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Key.Offset is the start offset, chunk.offset is the current read position

lightning/restore/restore.go Outdated Show resolved Hide resolved
@lance6716
Copy link
Contributor

LGTM

CI fail since TiDB keep the format d: 18446744073709551616.0 back

@lance6716 lance6716 added the status/LGT1 One reviewer already commented LGTM (LGTM1) label Jan 11, 2021
Copy link
Collaborator

@kennytm kennytm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest LGTM

lightning/restore/restore.go Outdated Show resolved Hide resolved
@kennytm
Copy link
Collaborator

kennytm commented Jan 27, 2021

/lgtm

@glorv glorv added status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2) and removed status/LGT1 One reviewer already commented LGTM (LGTM1) labels Jan 27, 2021
@glorv
Copy link
Contributor Author

glorv commented Jan 27, 2021

/merge

@ti-srebot
Copy link
Contributor

@glorv Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 1

@lance6716
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

@lance6716 Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 1

@lance6716
Copy link
Contributor

/lgtm

@lance6716
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 6c382e2 into master Jan 27, 2021
@glorv glorv deleted the import-progress branch January 27, 2021 08:30
glorv added a commit that referenced this pull request Jan 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CanMerge status/LGT2 Two reviewers already commented LGTM, ready for merge (LGTM2)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants