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.66567% with 209 lines in your changes missing coverage. Please review.

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

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57148        +/-   ##
================================================
+ Coverage   72.8369%   75.6978%   +2.8609%     
================================================
  Files          1677       1730        +53     
  Lines        464141     482410     +18269     
================================================
+ Hits         338066     365174     +27108     
+ Misses       105185      95099     -10086     
- Partials      20890      22137      +1247

Flag	Coverage Δ
integration	`50.2183% <14.8425%> (?)`
unit	`73.1205% <66.7166%> (+0.8769%)`	⬆️

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

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

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

* 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: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.

lance6716

approve for domain part

ti-chi-bot · 2024-12-03T02:03:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Benjamin2037, henrybw, lance6716, xhebox

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

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 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

wddevries and others added 18 commits November 27, 2024 13:32

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

lance6716 approved these changes Dec 3, 2024

View reviewed changes

ti-chi-bot bot added the approved label Dec 3, 2024

ti-chi-bot bot merged commit cb408e0 into pingcap:master Dec 3, 2024
27 checks passed

wddevries deleted the rebase_to_master branch December 3, 2024 22:02

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

wddevries commented Nov 6, 2024

tiprow bot commented Nov 6, 2024

codecov bot commented Nov 6, 2024 •

edited

Loading

hawkingrei commented Nov 6, 2024

hawkingrei Nov 6, 2024

wddevries Nov 20, 2024

henrybw left a comment

bb7133 commented Nov 15, 2024

wddevries commented Nov 15, 2024

tiprow bot commented Nov 15, 2024

xhebox left a comment

xhebox Nov 18, 2024

henrybw left a comment

ti-chi-bot bot commented Nov 18, 2024

Benjamin2037 commented Nov 20, 2024 •

edited

Loading

D3Hunter Nov 20, 2024

wddevries Nov 20, 2024

D3Hunter left a comment •

edited

Loading

D3Hunter commented Nov 20, 2024

xhebox commented Nov 20, 2024

D3Hunter commented Nov 20, 2024

Benjamin2037 left a comment

ti-chi-bot bot commented Dec 2, 2024

lance6716 left a comment

ti-chi-bot bot commented Dec 3, 2024

workload repo: Merge Workload Repository into master #57148

workload repo: Merge Workload Repository into master #57148

Conversation

wddevries commented Nov 6, 2024

What problem does this PR solve?

Check List

Release note

tiprow bot commented Nov 6, 2024

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

hawkingrei commented Nov 6, 2024

hawkingrei Nov 6, 2024

Choose a reason for hiding this comment

wddevries Nov 20, 2024

Choose a reason for hiding this comment

henrybw left a comment

Choose a reason for hiding this comment

bb7133 commented Nov 15, 2024

wddevries commented Nov 15, 2024

tiprow bot commented Nov 15, 2024

xhebox left a comment

Choose a reason for hiding this comment

xhebox Nov 18, 2024

Choose a reason for hiding this comment

henrybw left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 18, 2024

Benjamin2037 commented Nov 20, 2024 • edited Loading

D3Hunter Nov 20, 2024

Choose a reason for hiding this comment

wddevries Nov 20, 2024

Choose a reason for hiding this comment

D3Hunter left a comment • edited Loading

Choose a reason for hiding this comment

D3Hunter commented Nov 20, 2024

xhebox commented Nov 20, 2024

D3Hunter commented Nov 20, 2024

Benjamin2037 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 2, 2024

[LGTM Timeline notifier]

lance6716 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 3, 2024

codecov bot commented Nov 6, 2024 •

edited

Loading

Benjamin2037 commented Nov 20, 2024 •

edited

Loading

D3Hunter left a comment •

edited

Loading