Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix/scalein pump monitor not reloaded #958

Merged
merged 8 commits into from
Dec 9, 2020
Merged

Fix/scalein pump monitor not reloaded #958

merged 8 commits into from
Dec 9, 2020

Conversation

9547
Copy link
Contributor

@9547 9547 commented Dec 1, 2020

What problem does this PR solve?

Fix #936

What is changed and how it works?

Check List

Tests

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

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release notes:

NONE

@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 1, 2020
@ti-chi-bot ti-chi-bot requested a review from nrc December 1, 2020 16:47
@codecov-io
Copy link

codecov-io commented Dec 1, 2020 •
edited
Loading

Codecov Report

Merging #958 (deea6cc) into master (3bf3ba8) will decrease coverage by 6.72%.
The diff coverage is 71.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #958      +/-   ##
==========================================
- Coverage   55.78%   49.06%   -6.73%     
==========================================
  Files         263      263              
  Lines       19522    19587      +65     
==========================================
- Hits        10891     9610    -1281     
- Misses       6903     8449    +1546     
+ Partials     1728     1528     -200
Flag Coverage Δ
cluster 31.91% <68.62%> (-11.41%) ⬇️
dm 24.24% <22.02%> (-0.06%) ⬇️
integrate 42.48% <71.59%> (-7.52%) ⬇️
playground 20.26% <ø> (ø)
tiup 16.80% <ø> (ø)
unittest 22.97% <0.00%> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
pkg/cluster/operation/scale_in.go 41.05% <ø> (-18.82%) ⬇️
pkg/cluster/manager.go 59.90% <60.86%> (-7.66%) ⬇️
pkg/cluster/task/update_meta.go 69.23% <91.89%> (+6.18%) ⬆️
components/cluster/command/prune.go 75.00% <100.00%> (+17.85%) ⬆️
components/dm/task/update_dm_meta.go 85.29% <100.00%> (+3.15%) ⬆️
pkg/cluster/template/config/config.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/cluster/template/scripts/scripts.go 0.00% <0.00%> (-100.00%) ⬇️
pkg/cluster/template/scripts/tiflash.go 0.00% <0.00%> (-79.60%) ⬇️
components/cluster/command/check.go 6.37% <0.00%> (-72.82%) ⬇️
pkg/cluster/task/limits.go 0.00% <0.00%> (-68.75%) ⬇️
... and 58 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bf3ba8...deea6cc. Read the comment docs.

@9547 9547 marked this pull request as draft December 1, 2020 17:20
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2020
@nrc nrc removed their request for review December 1, 2020 20:19
@9547 9547 marked this pull request as ready for review December 2, 2020 00:10
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 2, 2020
lucklove
lucklove reviewed Dec 8, 2020
View reviewed changes
pkg/cluster/manager.go

var regenConfigTasks []task.Task
deletedNodes := set.NewStringSet(nodes...)
topo.IterInstance(func(instance spec.Instance) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a function such as RefreshConfig to build the regenConfigTasks? I found that there is a lot of redundant code: https://github.com/pingcap/tiup/pull/958/files#diff-5609f7b865f14ce911938ab0a8f21caa07446244d9c106a5df27ba9c08cf80e6R1386

Notice: there are many other places use the same logic, we can replace that logic with just a function call:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this PR contains 200+ lines modified, IMO, I'll trigger another PR to do the refactor work. BTW the file manager.go is too large(~ 3000 loc), I want to split this file into some small files If you agree with it. :)

Copy link
Member

Choose a reason for hiding this comment

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

It's very nice if you can split the manager.go, I want to do that days ago.

I think we can add a package (eg. tiup/pkg/cluster/manager) which contains several .go files, each of them has different function (eg. split by Deploy, Start, 'ScaleOut` .etc.)

@9547 9547 force-pushed the fix/scalein-pump-monitor-notreload branch from deea6cc to f6df856 Compare December 8, 2020 16:27
@lucklove
Copy link
Member

lucklove commented Dec 9, 2020

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 9, 2020
@lucklove
Copy link
Member

lucklove commented Dec 9, 2020

/merge

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 9, 2020
@ti-chi-bot
Copy link
Member

Can merge label has been added.

Git tree hash: 6238d42

@ti-chi-bot ti-chi-bot merged commit a9370cd into pingcap:master Dec 9, 2020
@9547 9547 mentioned this pull request Dec 9, 2020
Merged
@AstroProfundis AstroProfundis changed the title Fix/scalein pump monitor notreload Fix/scalein pump monitor not reloaded Dec 15, 2020
@AstroProfundis AstroProfundis added the category/monitoring Categorizes issue or PR related to monitoring components. label Dec 15, 2020
@9547 9547 deleted the fix/scalein-pump-monitor-notreload branch April 6, 2021 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucklove lucklove lucklove left review comments

Assignees
No one assigned
Labels
category/monitoring Categorizes issue or PR related to monitoring components. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

scale-in pump/drainer action，prometheus do not update target
6 participants
@9547 @codecov-io @lucklove @ti-chi-bot @AstroProfundis @jsvisa