workload repo: Merge Workload Repository into master #57148

wddevries · 2024-11-06T01:00:20Z

What problem does this PR solve?

Issue Number: ref #57147

Problem Summary:

This pull request merges in the Workload Repository code from the Serverless project. The project is not complete yet, and more testing remains to be done.

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

tiprow · 2024-11-06T01:00:37Z

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

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 · 2024-11-06T01:10:55Z

Codecov Report

Attention: Patch coverage is 68.51574% with 210 lines in your changes missing coverage. Please review.

Project coverage is 74.7192%. Comparing base (d0de86b) to head (5368a64).
Report is 34 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57148        +/-   ##
================================================
+ Coverage   72.8369%   74.7192%   +1.8823%     
================================================
  Files          1677       1728        +51     
  Lines        464141     472889      +8748     
================================================
+ Hits         338066     353339     +15273     
+ Misses       105185      97319      -7866     
- Partials      20890      22231      +1341

Flag	Coverage Δ
integration	`49.1759% <14.8425%> (?)`
unit	`72.2236% <66.5667%> (-0.0201%)`	⬇️

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

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`61.1996% <ø> (+15.5050%)`	⬆️

hawkingrei · 2024-11-06T05:44:36Z

/ok-to-test

hawkingrei · 2024-11-06T05:49:25Z

cmd/tidb-server/BUILD.bazel

@@ -10,6 +10,7 @@ go_library(
        "//pkg/config",


Can you add some test cases for it?

I am not sure exactly what you mean, but we are in the process of creating tests for this code.

henrybw

The rest of the changes LGTM.

pkg/domain/repository/worker.go

bb7133 · 2024-11-15T02:33:41Z

/retest

pkg/domain/repository/worker.go

wddevries · 2024-11-15T07:17:41Z

/test pull-integration-ddl-test

tiprow · 2024-11-15T07:18:03Z

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

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

/test pull-integration-ddl-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.

xhebox

LGTM

xhebox · 2024-11-18T02:40:32Z

pkg/ddl/executor.go

@@ -4070,6 +4070,9 @@ var systemTables = map[string]struct{}{
 }

 func isUndroppableTable(schema, table string) bool {
+	if schema == "workload_schema" {


Maybe we should use const in repository package. Removing depedency is great, but it will also be convenient when we want to know all occurences of schema table name by searching workloadSchemaCIStr.

henrybw

LGTM

ti-chi-bot · 2024-11-18T20:03:33Z

@henrybw: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

LGTM

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/test-infra repository.

Benjamin2037 · 2024-11-20T02:11:12Z

Can we move repository package out of Domain PKG，since I do not think it is related to Domain and we planing to further refactor of Domain definition and interface granularly. It is better that repository's implement code should be organized in another proper place. Thanks!

D3Hunter · 2024-11-20T02:21:22Z

pkg/domain/repository/BUILD.bazel

prefer name this pkg as workloadrepo, repository is too generic

We will move the package. I'll discuss the name with the others, but I am not opposed to renaming it.

D3Hunter

and I believe this is a feature, not enhancement, need PM approve first

D3Hunter · 2024-11-20T02:22:29Z

/hold

xhebox · 2024-11-20T02:24:45Z

and I believe this is a feature, not enhancement, need PM approve first

It is approved.

/unhold

D3Hunter · 2024-11-20T02:29:40Z

@xhebox please ask if it's ok to unhold before doing it, i hold it also for the comments of myself and @Benjamin2037

* repository: sample and snapshot threads Signed-off-by: xhe <xw897002528@gmail.com> * fix Signed-off-by: xhe <xw897002528@gmail.com> * fix Signed-off-by: xhe <xw897002528@gmail.com> * address comments Signed-off-by: xhe <xw897002528@gmail.com> * address comments Signed-off-by: xhe <xw897002528@gmail.com> * address coments Signed-off-by: xhe <xw897002528@gmail.com> * address coments Signed-off-by: xhe <xw897002528@gmail.com> * address comments Signed-off-by: xhe <xw897002528@gmail.com> * address comments Signed-off-by: xhe <xw897002528@gmail.com> * address comments Signed-off-by: xhe <xw897002528@gmail.com> --------- Signed-off-by: xhe <xw897002528@gmail.com>

…als. (pingcap#1317) * Add global variables to control sampling intervals in the Workload Repository. * Return error when etc client is not available. * Rework error messages. * Fix required comments. * Switch to using Ticker.Reset instead of restarting Fiber. * Use swap instead of load and store. * Make a couple of minor changes. * Fix issues

Signed-off-by: xhe <xw897002528@gmail.com>

* Wrap exit with a context.Context. * Remove getGlobalVar from Worker. * Seperate Repository from Domain. * Use RegisterSysVar instead of defining varibles in SysVar.go * Move samplingInterval and snapshotInterval to Worker. * Convert setRepositoryDest to a method of Worker. * Make Worker private to the repository package. * Add error handling in SetupRepository and disable repository. * Change to statically allocated worker class and enable repository. * Don't wait on exit channel for domain. * Stop the repository from cleanup in main.go. * Fix nits and move the error check for etcd later in start(). * Add comments on public methods. * Fix error message format.

Signed-off-by: xhe <xw897002528@gmail.com>

…t test.

)

Benjamin2037

LGTM

ti-chi-bot · 2024-12-02T07:17:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Benjamin2037, henrybw, xhebox
Once this PR has been reviewed and has the lgtm label, please assign wjhuang2016 for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

~~OWNERS~~ [Benjamin2037,xhebox]
~~pkg/ddl/OWNERS~~ [Benjamin2037]
pkg/domain/OWNERS

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

ti-chi-bot · 2024-12-02T07:17:21Z

[LGTM Timeline notifier]

Timeline:

2024-11-18 02:40:53.091740391 +0000 UTC m=+842415.282609373: ☑️ agreed by xhebox.
2024-12-02 07:17:20.623003209 +0000 UTC m=+1052828.242657725: ☑️ agreed by Benjamin2037.

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 6, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Nov 6, 2024

hawkingrei reviewed Nov 6, 2024

View reviewed changes

henrybw reviewed Nov 7, 2024

View reviewed changes

pkg/domain/repository/worker.go Outdated Show resolved Hide resolved

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2024

wddevries force-pushed the rebase_to_master branch from 9a36a9b to b4c28ff Compare November 8, 2024 18:05

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

wddevries force-pushed the rebase_to_master branch 2 times, most recently from c79b50e to 2c50ab3 Compare November 13, 2024 05:56

xhebox reviewed Nov 15, 2024

View reviewed changes

pkg/domain/repository/worker.go Outdated Show resolved Hide resolved

xhebox approved these changes Nov 18, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 18, 2024

henrybw approved these changes Nov 18, 2024

View reviewed changes

D3Hunter reviewed Nov 20, 2024

View reviewed changes

D3Hunter changed the title ~~repository: Merge Workload Repository into master~~ workload repo: Merge Workload Repository into master Nov 20, 2024

D3Hunter reviewed Nov 20, 2024

View reviewed changes

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2024

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 20, 2024

xhebox and others added 22 commits November 27, 2024 13:32

repository: expose variables for users (pingcap#1353)

6601452

Signed-off-by: xhe <xw897002528@gmail.com>

Add code to start owner compaign.

5c34c72

Fix bazel scripts.

c440359

Use any instead of interface{} in worker.go.

d77e8fd

Fix owner campaign patch.

7783930

Remove unused error return value from Stop()

3499a36

Use tagged switch on dst.

920ed62

Check error result from res.Close() in worker.runQuery().

91fd463

Rename unused parameters to _.

2ed51fa

Remove unused method receiver 'w' from several functions.

1074f4c

Fix type in description of comment for WR_VER field of HIST_SNAPSHOT.

b280791

Cancel() was removed from owner.Manger. Use Close() instead.

0150dc3

Remove unneeded return statement and unused method receivers.

4a3a4a9

repository: unit test example (pingcap#1379)

94a5619

Signed-off-by: xhe <xw897002528@gmail.com>

Fix example tests.

28e5f18

Rework worker.stop() to fix race.

0027670

Set owner.ManagerSessionTTL instead of starting a campaign in the uni…

c0d62d7

…t test.

Move repository to pkg/util and rename it to workloadrepo. (pingcap#1418

4719db2

)

Rewrite some loops to use range.

5368a64

hawkingrei force-pushed the rebase_to_master branch from 17304bf to 5368a64 Compare November 27, 2024 05:32

xhebox requested a review from D3Hunter November 27, 2024 10:29

D3Hunter requested a review from Benjamin2037 November 27, 2024 15:08

Benjamin2037 approved these changes Dec 2, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

workload repo: Merge Workload Repository into master #57148

workload repo: Merge Workload Repository into master #57148

workload repo: Merge Workload Repository into master #57148

Are you sure you want to change the base?

workload repo: Merge Workload Repository into master #57148

Conversation

What problem does this PR solve?

Check List

Release note

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[LGTM Timeline notifier]