executor: add remain time for showAnalyzeStatus #43866

hawkingrei · 2023-05-16T06:52:57Z

What problem does this PR solve?

Issue Number: close #44033

Problem Summary:

What is changed and how it works?

Check List

Tests

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

Side effects

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

Documentation

Release note

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

None

ti-chi-bot · 2023-05-16T06:52:58Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

qw4990
xuyifangreeneyes

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

executor/infoschema_reader.go

xuyifangreeneyes · 2023-05-24T09:23:23Z

executor/infoschema_reader.go

 // setDataForAnalyzeStatus gets all the analyze jobs.
 func (e *memtableRetriever) setDataForAnalyzeStatus(sctx sessionctx.Context) (err error) {
-	e.rows, err = dataForAnalyzeStatusHelper(sctx)
+	e.rows, err = dataForAnalyzeStatusHelper(sctx, false)


I think it is also ok to add those fields for information_schema.analyze_status.

Yes, As the changes are extensive, it may be difficult to review all of them within a single pull request. information_schema.analyze_status will be in the next PR.

hawkingrei · 2023-05-25T02:12:27Z

/retest

xuyifangreeneyes · 2023-05-25T03:10:44Z

executor/infoschema_reader.go

+			pt := tb.Meta().GetPartitionInfo()
+			tid = pt.GetPartitionIDByName(partitionName)
+		} else {
+			statsTable := statistics.PseudoTable(tb.Meta())


Maybe here we can call domain.GetDomain(sctx).StatsHandle().GetPartitionStats(tbl.Meta(), tid) for both partition and non-partitioned table. (*Handle).GetTableStats is just a thin wrapper of (*Handle).GetPartitionStats.

OK, I have refactored it.

xuyifangreeneyes · 2023-05-25T06:23:14Z

executor/infoschema_reader.go

+				pt := meta.GetPartitionInfo()
+				tid = pt.GetPartitionIDByName(partitionName)
+				statsTbl = statsHandle.GetPartitionStats(meta, tid)
+			}


It seems that we need to exchange the two branches.

xuyifangreeneyes · 2023-05-25T06:31:43Z

executor/internal/util/util.go

+		regionStats.Count = 1
+		// Set a very large approximate count.
+		regionStats.StorageKeys = 1000000
+	})


Will we add some tests using the failpoint?

Yes, I will add tests by the other PR. This failpoint is from the old test.

I hope it can be merged and included in today's nightly version so that I can test it manually.

xuyifangreeneyes

rest LGTM

executor/infoschema_reader.go

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

hawkingrei · 2023-05-25T11:19:12Z

/retest

hawkingrei · 2023-05-25T11:47:11Z

/merge

ti-chi-bot · 2023-05-25T11:47:15Z

This pull request has been accepted and is ready to merge.

Commit hash: da40a2c

hawkingrei · 2023-05-25T12:21:40Z

/retest

chrysan · 2023-06-15T10:38:53Z

executor/infoschema_reader.go

+			}
+		}
+		if tid > 0 && totalCnt == 0 {
+			totalCnt, _ = internalutil.GetApproximateTableCountFromStorage(sctx, tid, dbName, tableName, partitionName)


This func will be called at getAdjustedSampleRate when we do the first analyze, and called again when show analyze status, right? Can we reuse, like persisting the ApproximateTableCount somewhere?

UI will call showAnalyzeStatus many times to get fresh status. I'm afraid there will be frequent calls to internalutil.GetApproximateTableCountFromStorage.

chrysan · 2023-06-15T10:44:38Z

executor/infoschema_reader.go

+		duration = 1 * time.Second
+	}
+	i := float64(remainLine) * duration.Seconds() / float64(processedRows)
+	persentage := float64(processedRows) / float64(totalCnt)


typo: persentage-> percentage

chrysan · 2023-06-15T10:54:02Z

executor/infoschema_reader.go

+				instance,           // INSTANCE
+				procID,             // PROCESS_ID
+				remainDurationStr,  // REMAINING_SECONDS
+				progressStr,        // PROGRESS


It's better to be a numeric type for UI to easily parse and aggregate.

hawkingrei force-pushed the add_remain_time_for_analyze branch 3 times, most recently from 8793f1b to 01b9751 Compare May 17, 2023 07:23

xuyifangreeneyes self-requested a review May 17, 2023 08:47

hawkingrei force-pushed the add_remain_time_for_analyze branch from 654e989 to 6fe1de5 Compare May 19, 2023 09:09

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/needs-linked-issue labels May 19, 2023

xuyifangreeneyes reviewed May 24, 2023

View reviewed changes

executor/infoschema_reader.go Outdated Show resolved Hide resolved

xuyifangreeneyes reviewed May 24, 2023

View reviewed changes

executor/infoschema_reader.go Outdated Show resolved Hide resolved

xuyifangreeneyes reviewed May 24, 2023

View reviewed changes

hawkingrei force-pushed the add_remain_time_for_analyze branch 3 times, most recently from 4608f72 to be97a66 Compare May 24, 2023 16:35

xuyifangreeneyes reviewed May 25, 2023

View reviewed changes

hawkingrei force-pushed the add_remain_time_for_analyze branch from 8e57c73 to 5804ddb Compare May 25, 2023 03:48

xuyifangreeneyes reviewed May 25, 2023

View reviewed changes

xuyifangreeneyes approved these changes May 25, 2023

View reviewed changes

ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 25, 2023

hawkingrei requested a review from qw4990 May 25, 2023 06:52

qw4990 reviewed May 25, 2023

View reviewed changes

executor/infoschema_reader.go Outdated Show resolved Hide resolved

hawkingrei added 14 commits May 25, 2023 19:12

update

375c603

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

5b2a420

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

37a703d

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

test

9a4727f

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

3841ad5

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

906a5da

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

edd0939

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

1bf44aa

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

fdf73cd

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

1cc7d1d

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

c5aff05

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

update

4886b4f

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

adding test

7115c75

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

adding test

da40a2c

Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>

hawkingrei force-pushed the add_remain_time_for_analyze branch from 7b522f8 to da40a2c Compare May 25, 2023 11:12

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2023

qw4990 approved these changes May 25, 2023

View reviewed changes

ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 25, 2023

qw4990 added the sig/planner SIG: Planner label May 25, 2023

ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label May 25, 2023

ti-chi-bot bot merged commit 557167e into pingcap:master May 25, 2023
11 checks passed

hawkingrei deleted the add_remain_time_for_analyze branch May 25, 2023 16:05

chrysan reviewed Jun 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: add remain time for showAnalyzeStatus #43866

executor: add remain time for showAnalyzeStatus #43866

hawkingrei commented May 16, 2023 •

edited

ti-chi-bot bot commented May 16, 2023 •

edited

xuyifangreeneyes May 24, 2023

hawkingrei May 25, 2023

hawkingrei commented May 25, 2023

xuyifangreeneyes May 25, 2023

hawkingrei May 25, 2023

xuyifangreeneyes May 25, 2023

hawkingrei May 25, 2023

xuyifangreeneyes May 25, 2023

hawkingrei May 25, 2023

hawkingrei May 25, 2023

xuyifangreeneyes left a comment

hawkingrei commented May 25, 2023

hawkingrei commented May 25, 2023

ti-chi-bot bot commented May 25, 2023

hawkingrei commented May 25, 2023

chrysan Jun 15, 2023

chrysan Jun 15, 2023

chrysan Jun 15, 2023

chrysan Jun 15, 2023 •

edited

executor: add remain time for showAnalyzeStatus #43866

executor: add remain time for showAnalyzeStatus #43866

Conversation

hawkingrei commented May 16, 2023 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented May 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkingrei commented May 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuyifangreeneyes left a comment

Choose a reason for hiding this comment

hawkingrei commented May 25, 2023

hawkingrei commented May 25, 2023

ti-chi-bot bot commented May 25, 2023

hawkingrei commented May 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrysan Jun 15, 2023 • edited

Choose a reason for hiding this comment

hawkingrei commented May 16, 2023 •

edited

ti-chi-bot bot commented May 16, 2023 •

edited

chrysan Jun 15, 2023 •

edited