tools: fix the inconsistency of available space #5670

rleungx · 2022-11-02T10:51:07Z

Signed-off-by: Ryan Leung rleungx@gmail.com

What problem does this PR solve?

Issue Number: Ref #5468

What is changed and how does it work?

Prevent the following situation from happening.

Check List

Tests

Manual test

Release note

None

ti-chi-bot · 2022-11-02T10:51:08Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

CabinfeverB
lhy1024

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.

codecov · 2022-11-02T10:59:05Z

Codecov Report

Base: 75.82% // Head: 75.82% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (9b162a3) compared to base (fcfcda3).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5670   +/-   ##
=======================================
  Coverage   75.82%   75.82%           
=======================================
  Files         329      329           
  Lines       32682    32683    +1     
=======================================
+ Hits        24781    24783    +2     
  Misses       5781     5781           
+ Partials     2120     2119    -1

Flag	Coverage Δ
unittests	`75.82% <100.00%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
server/schedulers/hot_region.go	`85.03% <100.00%> (+0.01%)`	⬆️
pkg/tempurl/tempurl.go	`45.00% <0.00%> (-15.00%)`	⬇️
pkg/metricutil/metricutil.go	`82.75% <0.00%> (-10.35%)`	⬇️
server/storage/kv/etcd_kv.go	`79.71% <0.00%> (-5.80%)`	⬇️
server/tso/allocator_manager.go	`64.51% <0.00%> (-3.07%)`	⬇️
pkg/dashboard/adapter/manager.go	`79.31% <0.00%> (-2.30%)`	⬇️
server/region_syncer/server.go	`86.26% <0.00%> (-1.10%)`	⬇️
server/storage/hot_region_storage.go	`78.14% <0.00%> (-1.10%)`	⬇️
server/region_syncer/client.go	`85.82% <0.00%> (-0.75%)`	⬇️
server/tso/tso.go	`70.78% <0.00%> (-0.57%)`	⬇️
... and 18 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

bufferflies · 2022-11-03T03:01:09Z

tools/pd-simulator/simulator/cases/region_split.go

+			ID:       uint64(i),
+			Status:   metapb.StoreState_Up,
+			Capacity: 1 * units.TiB,
+			Version:  "2.1.0",


Do we need to change the version in master？plz,see @lhy1024

I think we can do it in another pr #5672

bufferflies · 2022-11-03T03:14:04Z

tools/pd-simulator/simulator/cases/diagnose_rule.go

+			ID:       id,
+			Status:   metapb.StoreState_Up,
+			Capacity: 1000 * units.GiB,
+			Version:  "2.1.0",


how about remove this Version？

I think it's necessary.

bufferflies · 2022-11-03T03:19:17Z

BTW, I think it's very nice to use config file to describe our loads not hard code. And the store status can be remvoed from the case like state, available ,version .

bufferflies

rest LGTM

lhy1024 · 2022-11-03T03:16:32Z

tools/pd-simulator/simulator/cases/diagnose_rule.go

+			ID:       id,
+			Status:   metapb.StoreState_Up,
+			Capacity: 1000 * units.GiB,
+			Version:  "2.1.0",


I think it's necessary.

lhy1024 · 2022-11-03T03:18:00Z

tools/pd-simulator/simulator/cases/redundant_balance_region.go

-				ID:        IDAllocator.nextID(),
-				Status:    metapb.StoreState_Up,
-				Capacity:  1 * units.TiB,
-				Available: 980 * units.GiB,


we still need different Available, 980 * units.GiB and 1 * units.TiB

Does this case still work? I test it and there are only balance leader operator.

BTW, I think this can be solved through redistributing regions.

Maybe we need to use different used size?

Maybe we can put ExtraUsedSpace put into case to update store status

Maybe we can put ExtraUsedSpace put into case to update store status

I think so.

I plan to simplify the case because we need to recompile it every time. Currently, it can also be done by using ExtraUsedSpace in the config file.

If so, I think we should provide default value

lhy1024 · 2022-11-03T03:18:54Z

tools/pd-simulator/simulator/cases/region_split.go

+			ID:       uint64(i),
+			Status:   metapb.StoreState_Up,
+			Capacity: 1 * units.TiB,
+			Version:  "2.1.0",


I think we can do it in another pr #5672

lhy1024 · 2022-11-03T07:03:11Z

tools/pd-simulator/simulator/cases/redundant_balance_region.go

-				ID:        IDAllocator.nextID(),
-				Status:    metapb.StoreState_Up,
-				Capacity:  1 * units.TiB,
-				Available: 980 * units.GiB,


Maybe we need to use different used size?

lhy1024 · 2022-11-03T07:03:43Z

tools/pd-simulator/simulator/cases/add_nodes.go

-			Status:    metapb.StoreState_Up,
-			Capacity:  1 * units.TiB,
-			Available: 900 * units.GiB,
-			Version:   "2.1.0",


I think we need some test to ensure release version is available for these cases

The store version can be configured through config file.

whar is its default version

lhy1024

the rest LGTM

Signed-off-by: Ryan Leung <rleungx@gmail.com>

lhy1024 · 2022-11-14T03:29:27Z

tools/pd-simulator/simulator/cases/redundant_balance_region.go

-				ID:        IDAllocator.nextID(),
-				Status:    metapb.StoreState_Up,
-				Capacity:  1 * units.TiB,
-				Available: 980 * units.GiB,


If so, I think we should provide default value

rleungx · 2022-11-14T03:46:01Z

/merge

ti-chi-bot · 2022-11-14T03:46:03Z

@rleungx: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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 ti-community-infra/tichi repository.

ti-chi-bot · 2022-11-14T03:46:04Z

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

Commit hash: 9b162a3

ti-chi-bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 2, 2022

ti-chi-bot requested review from lhy1024 and nolouch November 2, 2022 10:51

rleungx force-pushed the available branch from 1f9d3d4 to 7ce82d2 Compare November 3, 2022 02:43

rleungx changed the title *: fix the conflict of available tools: fix the conflict of available Nov 3, 2022

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 3, 2022

rleungx marked this pull request as ready for review November 3, 2022 02:45

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

rleungx changed the title ~~tools: fix the conflict of available~~ tools: fix the inconsistency of available space Nov 3, 2022

bufferflies reviewed Nov 3, 2022

View reviewed changes

lhy1024 reviewed Nov 3, 2022

View reviewed changes

rleungx force-pushed the available branch from 9a9777e to 0fc0313 Compare November 3, 2022 05:54

lhy1024 reviewed Nov 3, 2022

View reviewed changes

rleungx mentioned this pull request Nov 3, 2022

tools: support triggering an event through HTTP API #5677

Merged

CabinfeverB approved these changes Nov 11, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 11, 2022

rleungx requested review from lhy1024 and bufferflies November 11, 2022 06:09

lhy1024 reviewed Nov 11, 2022

View reviewed changes

rleungx requested a review from lhy1024 November 14, 2022 02:48

fix the conflict of available

ce24b84

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx added 2 commits November 14, 2022 11:39

address comments

f28b999

Signed-off-by: Ryan Leung <rleungx@gmail.com>

address the comment

9b162a3

Signed-off-by: Ryan Leung <rleungx@gmail.com>

rleungx force-pushed the available branch from 29812d4 to 9b162a3 Compare November 14, 2022 03:40

lhy1024 approved these changes Nov 14, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 14, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 14, 2022

ti-chi-bot merged commit e6eef7c into tikv:master Nov 14, 2022

rleungx deleted the available branch November 14, 2022 03:54

lhy1024 mentioned this pull request Nov 15, 2022

simulator: change the version from 2.1.0 #5672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: fix the inconsistency of available space #5670

tools: fix the inconsistency of available space #5670

rleungx commented Nov 2, 2022 •

edited

Loading

ti-chi-bot commented Nov 2, 2022 •

edited

Loading

codecov bot commented Nov 2, 2022 •

edited

Loading

bufferflies Nov 3, 2022

lhy1024 Nov 3, 2022

bufferflies Nov 3, 2022

lhy1024 Nov 3, 2022

bufferflies commented Nov 3, 2022

bufferflies left a comment

lhy1024 Nov 3, 2022

lhy1024 Nov 3, 2022

rleungx Nov 3, 2022

rleungx Nov 3, 2022

lhy1024 Nov 3, 2022

CabinfeverB Nov 7, 2022

lhy1024 Nov 11, 2022

rleungx Nov 14, 2022

lhy1024 Nov 14, 2022

lhy1024 Nov 3, 2022

lhy1024 Nov 3, 2022

lhy1024 Nov 3, 2022

rleungx Nov 11, 2022

lhy1024 Nov 11, 2022

lhy1024 left a comment

lhy1024 Nov 14, 2022

rleungx commented Nov 14, 2022

ti-chi-bot commented Nov 14, 2022

ti-chi-bot commented Nov 14, 2022

tools: fix the inconsistency of available space #5670

tools: fix the inconsistency of available space #5670

Conversation

rleungx commented Nov 2, 2022 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Nov 2, 2022 • edited Loading

codecov bot commented Nov 2, 2022 • edited Loading

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

bufferflies commented Nov 3, 2022

bufferflies left a 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

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

lhy1024 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx commented Nov 14, 2022

ti-chi-bot commented Nov 14, 2022

ti-chi-bot commented Nov 14, 2022

rleungx commented Nov 2, 2022 •

edited

Loading

ti-chi-bot commented Nov 2, 2022 •

edited

Loading

codecov bot commented Nov 2, 2022 •

edited

Loading