workload-based learning: save the metrics and create new workload_values table by elsa0520 · Pull Request #59126 · pingcap/tidb

elsa0520 · 2025-01-22T09:43:09Z

What problem does this PR solve?

Issue Number: ref #58131

Problem Summary:

What changed and how does it work?

This PR implement the part of producer table cost feature:

Implement the save table cost values into workload_values table
Create the new table workload_values . This is a system table used for all of workload-learning related values.
Upgrade the meta version to "upgradeToVer242" . This version will also be used for following workload system tables before the new version of tidb released.

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

Release note

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

None

ti-chi-bot · 2025-01-22T09:43:12Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tiprow · 2025-01-22T09:43:29Z

Hi @elsa0520. 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.

Details

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.

codecov · 2025-02-11T09:11:40Z

Codecov Report

Attention: Patch coverage is 31.96721% with 83 lines in your changes missing coverage. Please review.

Project coverage is 74.9220%. Comparing base (eaa39c9) to head (84d0688).
Report is 31 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #59126        +/-   ##
================================================
+ Coverage   72.9996%   74.9220%   +1.9224%     
================================================
  Files          1693       1743        +50     
  Lines        468156     482488     +14332     
================================================
+ Hits         341752     361490     +19738     
+ Misses       105370      98413      -6957     
- Partials      21034      22585      +1551

Flag	Coverage Δ
integration	`48.8804% <4.0983%> (?)`
unit	`72.3743% <31.9672%> (+0.1641%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`60.9408% <ø> (+15.4803%)`	⬆️

elsa0520 · 2025-02-11T09:50:15Z

/test unit-test

tiprow · 2025-02-11T09:50:37Z

@elsa0520: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test unit-test

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.

ghazalfamilyusa · 2025-02-11T17:24:49Z

General comments:

Change workloadBasedLearning to workloadLearning across the code
Why this code casn not be unit tested? Is there a mock that can be used to do the test?

pkg/session/bootstrap.go

pkg/statistics/analyze.go

elsa0520 · 2025-02-12T03:27:53Z

General comments:

Change workloadBasedLearning to workloadLearning across the code

Why this code casn not be unit tested? Is there a mock that can be used to do the test?

Oh, I just want to add the unit test after I finish the new system table of producer part. Because It is also another workload_jobs table will be raise in the next PR. So I didn't add the unit test in this PR

elsa0520 · 2025-02-12T03:28:17Z

General comments:

Change workloadBasedLearning to workloadLearning across the code

Why this code casn not be unit tested? Is there a mock that can be used to do the test?

OK, I will change it to workloadLearning

ghazalfamilyusa · 2025-02-12T19:26:39Z

General comments:

Change workloadBasedLearning to workloadLearning across the code

Why this code casn not be unit tested? Is there a mock that can be used to do the test?

Oh, I just want to add the unit test after I finish the new system table of producer part. Because It is also another workload_jobs table will be raise in the next PR. So I didn't add the unit test in this PR

I think it is still good to add unit tests incrementally.

ghazalfamilyusa

Remaining issues:

Remove comments not related to the PR
Add a unit test incrementally

ti-chi-bot · 2025-02-18T02:46:26Z

[LGTM Timeline notifier]

Timeline:

2025-02-11 17:25:17.621257095 +0000 UTC m=+377360.017479175: ✖️🔁 reset by ghazalfamilyusa.
2025-02-12 19:28:12.63896608 +0000 UTC m=+471135.035188142: ✖️🔁 reset by ghazalfamilyusa.
2025-02-13 16:19:12.840309993 +0000 UTC m=+546195.236532054: ☑️ agreed by ghazalfamilyusa.
2025-02-18 02:46:25.423341838 +0000 UTC m=+929427.819563900: ☑️ agreed by AilinKid.

elsa0520 · 2025-02-18T05:26:53Z

/test pull-lightning-integration-test

tiprow · 2025-02-18T05:27:16Z

@elsa0520: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test pull-lightning-integration-test

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.

elsa0520 · 2025-02-18T08:12:01Z

/test pull-lightning-integration-test

tiprow · 2025-02-18T08:12:23Z

@elsa0520: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/test pull-lightning-integration-test

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.

elsa0520 · 2025-02-18T08:13:22Z

/ok-to-test

D3Hunter · 2025-02-18T09:33:10Z

/approve

benmeadowcroft · 2025-02-18T19:16:07Z

pkg/session/bootstrap.go

+        primary key(module, name));`
+
+	// CreateWorkloadValuesTable is a table to store workload-based learning values for tidb.
+	CreateWorkloadValuesTable = `CREATE TABLE IF NOT EXISTS mysql.workload_values (


Is there a standard naming convention that we use for these new tables that are TiDB extensions? I see many other cases of these, non-MySQL, tables where we prefix the table name with tidb_, pd_, tikv_ or similar across the mysql and performance_schema schemas.

I am concerned that this kind of unprefixed table name could lead to future conflicts and compatibility issues if MySQL introduces this table name in the future.

Has been changed to tidb_workload_values

elsa0520 · 2025-02-19T05:28:57Z

/test pull-lightning-integration-test

elsa0520 · 2025-02-19T05:29:06Z

/test pull-br-integration-test

tiprow · 2025-02-19T05:29:20Z

@elsa0520: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test tidb_parser_test

Use /test all to run all jobs.

Details

In response to this:

/test pull-lightning-integration-test

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.

tiprow · 2025-02-19T05:29:27Z

@elsa0520: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test tidb_parser_test

Use /test all to run all jobs.

Details

In response to this:

/test pull-br-integration-test

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.

elsa0520 · 2025-02-19T08:50:05Z

/retest

ti-chi-bot · 2025-02-19T16:58:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, benmeadowcroft, D3Hunter, ghazalfamilyusa, qw4990, wjhuang2016, XuHuaiyu

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [AilinKid,D3Hunter,XuHuaiyu,qw4990,wjhuang2016]
~~pkg/domain/OWNERS~~ [D3Hunter,wjhuang2016]
~~pkg/executor/importer/OWNERS~~ [D3Hunter]
~~pkg/expression/OWNERS~~ [XuHuaiyu]
~~pkg/session/OWNERS~~ [benmeadowcroft]
~~pkg/statistics/OWNERS~~ [AilinKid,ghazalfamilyusa,qw4990]

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

elsa0520 · 2025-02-20T02:02:00Z

/retest

…ues table (pingcap#59126) ref pingcap#58131

elsa0520 force-pushed the learning-tablecost branch from fa51440 to ebe1657 Compare February 11, 2025 08:36

elsa0520 marked this pull request as ready for review February 11, 2025 08:36

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2025

elsa0520 changed the title ~~planner, workload-based learning: Save the metrics by batch in producer part~~ planner, workload-based learning: Step5 Save the metrics by batch in producer part and create new workload_values table Feb 11, 2025

ti-chi-bot bot removed do-not-merge/needs-tests-checked do-not-merge/needs-linked-issue labels Feb 11, 2025

elsa0520 changed the title ~~planner, workload-based learning: Step5 Save the metrics by batch in producer part and create new workload_values table~~ planner, workload-based learning: Save the metrics by batch in producer part and create new workload_values table Feb 11, 2025

elsa0520 changed the title ~~planner, workload-based learning: Save the metrics by batch in producer part and create new workload_values table~~ workload-based learning: save the metrics and create new workload_values table Feb 11, 2025

ghazalfamilyusa suggested changes Feb 11, 2025

View reviewed changes

pkg/session/bootstrap.go Show resolved Hide resolved

pkg/statistics/analyze.go Outdated Show resolved Hide resolved

pkg/statistics/analyze.go Outdated Show resolved Hide resolved

elsa0520 force-pushed the learning-tablecost branch from 89ffe40 to b41c17f Compare February 12, 2025 08:47

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. labels Feb 12, 2025

ghazalfamilyusa suggested changes Feb 12, 2025

View reviewed changes

ghazalfamilyusa approved these changes Feb 13, 2025

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Feb 13, 2025

fix channel

3421027

AilinKid approved these changes Feb 18, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 18, 2025

qw4990 approved these changes Feb 18, 2025

View reviewed changes

elsa0520 added 2 commits February 18, 2025 11:38

fix

a924d0b

fix

a012596

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Feb 18, 2025

benmeadowcroft reviewed Feb 18, 2025

View reviewed changes

change the system table name to tidb_workload_values

84d0688

benmeadowcroft approved these changes Feb 19, 2025

View reviewed changes

XuHuaiyu approved these changes Feb 19, 2025

View reviewed changes

ti-chi-bot bot added the approved label Feb 19, 2025

wjhuang2016 approved these changes Feb 19, 2025

View reviewed changes

ti-chi-bot bot merged commit 4db34bc into pingcap:master Feb 20, 2025
26 checks passed

zeminzhou pushed a commit to zeminzhou/tidb that referenced this pull request May 6, 2025

workload-based learning: save the metrics and create new workload_val…

dcb757b

…ues table (pingcap#59126) ref pingcap#58131

Conversation

elsa0520 commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Jan 22, 2025

Uh oh!

tiprow bot commented Jan 22, 2025

Uh oh!

codecov bot commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

elsa0520 commented Feb 11, 2025

Uh oh!

tiprow bot commented Feb 11, 2025

Uh oh!

ghazalfamilyusa commented Feb 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elsa0520 commented Feb 12, 2025

Uh oh!

elsa0520 commented Feb 12, 2025

Uh oh!

ghazalfamilyusa commented Feb 12, 2025

Uh oh!

ghazalfamilyusa left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Feb 18, 2025

[LGTM Timeline notifier]

Uh oh!

elsa0520 commented Feb 18, 2025

Uh oh!

tiprow bot commented Feb 18, 2025

Uh oh!

elsa0520 commented Feb 18, 2025

Uh oh!

tiprow bot commented Feb 18, 2025

Uh oh!

elsa0520 commented Feb 18, 2025

Uh oh!

D3Hunter commented Feb 18, 2025

Uh oh!

benmeadowcroft Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elsa0520 Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

elsa0520 commented Feb 19, 2025

Uh oh!

elsa0520 commented Feb 19, 2025

Uh oh!

tiprow bot commented Feb 19, 2025

Uh oh!

tiprow bot commented Feb 19, 2025

Uh oh!

elsa0520 commented Feb 19, 2025

Uh oh!

ti-chi-bot bot commented Feb 19, 2025

Uh oh!

elsa0520 commented Feb 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

elsa0520 commented Jan 22, 2025 •

edited

Loading

codecov bot commented Feb 11, 2025 •

edited

Loading

benmeadowcroft Feb 18, 2025 •

edited

Loading