*: add a API to clear tombstone store #1472

bradyjoestar · 2019-03-20T13:28:54Z

What problem does this PR solve?

#1468

Tests

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

Code changes

Has configuration change
Has HTTP API interfaces 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
Need to update the tidb-ansible repository
Need to be included in the release notes

merged

sre-bot · 2019-03-20T13:28:59Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

sre-bot · 2019-03-20T13:28:59Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

nolouch · 2019-03-21T08:21:52Z

Thanks! @bradyjoestar Can you add a command like

store clear [id]

in tools/pd-ctl.
and add some test in tests/cmd/pdctl_test.go

bradyjoestar · 2019-03-21T10:47:48Z

@nolunch sure.
I am a newbie so it may takes some time to read the testcode.

rleungx · 2019-03-21T11:21:54Z

server/cluster.go

@@ -557,6 +557,13 @@ func (c *RaftCluster) checkStores() {
 	}
 }

+// GetStores gets stores from cluster.


It seems this comment is not for this function?

rleungx · 2019-03-21T11:22:01Z

server/core/store.go

@@ -554,6 +554,15 @@ func (s *StoresInfo) GetMetaStores() []*metapb.Store {
 	return stores
 }

+// RemoveTombStoneStores remove all the record of tombstone Store.


Fixed!Thanks.

codecov-io · 2019-03-21T12:33:47Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f72cc23). Click here to learn what that means.
The diff coverage is 29.5%.

@@            Coverage Diff            @@
##             master    #1472   +/-   ##
=========================================
  Coverage          ?   67.13%           
=========================================
  Files             ?      158           
  Lines             ?    15383           
  Branches          ?        0           
=========================================
  Hits              ?    10328           
  Misses            ?     4112           
  Partials          ?      943

Impacted Files	Coverage Δ
server/core/store.go	`77.81% <0%> (ø)`
server/cluster.go	`75.06% <0%> (ø)`
server/cluster_info.go	`85.2% <0%> (ø)`
server/core/kv.go	`75.36% <0%> (ø)`
tools/pd-ctl/pdctl/ctl.go	`5% <0%> (ø)`
server/api/store.go	`63.3% <0%> (ø)`
server/schedule/basic_cluster.go	`88.67% <0%> (ø)`
server/api/router.go	`100% <100%> (ø)`
tools/pd-ctl/pdctl/command/store_command.go	`65.65% <94.44%> (ø)`

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 f72cc23...6816664. Read the comment docs.

nolouch · 2019-03-22T02:46:50Z

server/core/store.go

+func (s *StoresInfo) RemoveTombStoneRecords() {
+	for storeID, store := range s.stores {
+		if store.IsTombstone() {
+			delete(s.stores, storeID)


Only removed in the memory, restarting PD will appear again.

Fixed , Thanks !

nolouch · 2019-03-22T06:54:41Z

server/api/store.go

+		return
+	}
+
+	cluster.RemoveTombStoneRecords()


should handle the error.

nolouch · 2019-03-22T06:55:36Z

server/cluster.go

+			if err != nil {
+				return err
+			}
+		}


Better to add a log if delete successfully.

We want to know the deleted store through the log, so we also need to print the log if it is successfully deleted.

Got it. 👌

rleungx · 2019-03-22T07:30:30Z

server/api/router.go

@@ -64,6 +64,7 @@ func createRouter(prefix string, svr *server.Server) *mux.Router {
 	storeHandler := newStoreHandler(svr, rd)
 	router.HandleFunc("/api/v1/store/{id}", storeHandler.Get).Methods("GET")
 	router.HandleFunc("/api/v1/store/{id}", storeHandler.Delete).Methods("DELETE")
+	router.HandleFunc("/api/v1/store/remove-tombstone", storeHandler.RemoveTombStone).Methods("DELETE")


I think it's better to put it into stores instead of store?

Fine, I'll move it to storesHandler tonight. 👌

server/cluster.go

rleungx · 2019-03-22T07:33:47Z

tools/pd-ctl/pdctl/command/store_command.go

@@ -70,6 +70,15 @@ func NewSetStoreWeightCommand() *cobra.Command {
 	}
 }

+// NewRemoveTombStoneCommandFunc returns a weight subcommand of storeCmd.


rleungx · 2019-03-22T07:36:23Z

server/core/kv.go

@@ -114,6 +114,11 @@ func (kv *KV) SaveStore(store *metapb.Store) error {
 	return saveProto(kv.KVBase, kv.storePath(store.GetId()), store)
 }

+// DeleteStore delete one store from KV.


s/delete/deletes/g

Next time I will pay attention to it. 😁

Co-Authored-By: bradyjoestar <bradyjoestar@gmail.com>

nolouch

Cool!

nolouch · 2019-03-24T04:40:04Z

server/cluster.go

+			if err != nil {
+				return err
+			}
+		}


We want to know the deleted store through the log, so we also need to print the log if it is successfully deleted.

nolouch

LGTM

nolouch · 2019-03-25T03:55:15Z

/rebuild

zhouqiang-cl · 2019-03-25T06:10:29Z

/rebuild

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

bradyjoestar and others added 2 commits March 20, 2019 06:10

Merge pull request #1 from pingcap/master

b966728

merged

add remove tombstone api

3319a9e

rleungx added the contributor label Mar 21, 2019

Merge branch 'master' into issue-1468

e584283

rleungx reviewed Mar 21, 2019

View reviewed changes

fix ci bug

b75999f

swbsin added 3 commits March 21, 2019 06:05

add test and cmd

b8a9dde

fix ci bug

1840b3b

fix ci

52c06d8

nolouch reviewed Mar 22, 2019

View reviewed changes

swbsin added 2 commits March 21, 2019 22:24

clear tombstone in etcd

056d3f3

fix bug

253c9fb

nolouch reviewed Mar 22, 2019

View reviewed changes

add log and handle error

c15de7e

rleungx reviewed Mar 22, 2019

View reviewed changes

rleungx and others added 5 commits March 22, 2019 01:22

Update server/cluster.go

01b22a4

Co-Authored-By: bradyjoestar <bradyjoestar@gmail.com>

move remove-tombstone from store to stores

05e977c

fix comment

7da312b

gofmt

64c421a

fix comment

757d788

nolouch reviewed Mar 24, 2019

View reviewed changes

swbsin added 2 commits March 24, 2019 03:33

add success log

35e2cec

fix ci error

6816664

nolouch approved these changes Mar 25, 2019

View reviewed changes

rleungx approved these changes Mar 25, 2019

View reviewed changes

disksing merged commit 09aa89e into tikv:master Mar 25, 2019

bradyjoestar deleted the issue-1468 branch March 25, 2019 09:51

rleungx pushed a commit to rleungx/pd that referenced this pull request Aug 26, 2019

*: add a API to clear tombstone store (tikv#1472)

63bec49

rleungx mentioned this pull request Aug 26, 2019

*: add a API to clear tombstone store (#1472) #1705

Merged

sre-bot pushed a commit that referenced this pull request Aug 29, 2019

*: add a API to clear tombstone store (#1472) (#1705)

539f50e

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: add a API to clear tombstone store #1472

*: add a API to clear tombstone store #1472

bradyjoestar commented Mar 20, 2019

sre-bot commented Mar 20, 2019

sre-bot commented Mar 20, 2019

nolouch commented Mar 21, 2019 •

edited

Loading

bradyjoestar commented Mar 21, 2019

rleungx Mar 21, 2019

bradyjoestar Mar 21, 2019

rleungx Mar 21, 2019

bradyjoestar Mar 21, 2019

codecov-io commented Mar 21, 2019 •

edited

Loading

nolouch Mar 22, 2019 •

edited

Loading

bradyjoestar Mar 22, 2019

nolouch Mar 22, 2019

nolouch Mar 22, 2019

nolouch Mar 24, 2019

bradyjoestar Mar 24, 2019

rleungx Mar 22, 2019

bradyjoestar Mar 22, 2019

rleungx Mar 22, 2019

rleungx Mar 22, 2019

bradyjoestar Mar 22, 2019

nolouch left a comment

nolouch Mar 24, 2019

nolouch left a comment

nolouch commented Mar 25, 2019

zhouqiang-cl commented Mar 25, 2019

*: add a API to clear tombstone store #1472

*: add a API to clear tombstone store #1472

Conversation

bradyjoestar commented Mar 20, 2019

What problem does this PR solve?

sre-bot commented Mar 20, 2019

sre-bot commented Mar 20, 2019

nolouch commented Mar 21, 2019 • edited Loading

bradyjoestar commented Mar 21, 2019

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

codecov-io commented Mar 21, 2019 • edited Loading

Codecov Report

nolouch Mar 22, 2019 • edited Loading

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

nolouch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

nolouch commented Mar 25, 2019

zhouqiang-cl commented Mar 25, 2019

nolouch commented Mar 21, 2019 •

edited

Loading

codecov-io commented Mar 21, 2019 •

edited

Loading

nolouch Mar 22, 2019 •

edited

Loading