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.

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

*: add some store states. #775

Merged
merged 1 commit into from Sep 29, 2017

Conversation

Projects
None yet
5 participants
@disksing
Member

disksing commented Sep 28, 2017

Introduce the Disconnected state to identify stores that lost heartbeats for a short period of time, the LowSpace state to be shown in metrics as warning for potential disk space shortage risk.

@disksing disksing requested review from nolouch, siddontang and Connor1996 Sep 28, 2017

@nolouch

This comment has been minimized.

Show comment
Hide comment
@nolouch

nolouch Sep 29, 2017

Member

@tennix Does this change affect our k8s operator? seems the operator use the down API.

Member

nolouch commented Sep 29, 2017

@tennix Does this change affect our k8s operator? seems the operator use the down API.

@tennix

This comment has been minimized.

Show comment
Hide comment
@tennix

tennix Sep 29, 2017

Member

Yes, I've already opened an issue for this. Thanks for your reminder @nolouch

Member

tennix commented Sep 29, 2017

Yes, I've already opened an issue for this. Thanks for your reminder @nolouch

@disksing

This comment has been minimized.

Show comment
Hide comment
@disksing

disksing Sep 29, 2017

Member

Discussed with @tennix , they are going to update tidb-operator later after we merge this PR.
PTAL @huachaohuang @siddontang

Member

disksing commented Sep 29, 2017

Discussed with @tennix , they are going to update tidb-operator later after we merge this PR.
PTAL @huachaohuang @siddontang

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Sep 29, 2017

Member

LGTM

Member

siddontang commented Sep 29, 2017

LGTM

@nolouch nolouch merged commit 0946841 into master Sep 29, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-pd/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@nolouch nolouch deleted the disksing/metrics branch Sep 29, 2017

@@ -171,11 +178,16 @@ func (s *StoreInfo) GetUptime() time.Duration {
return 0
}
const defaultStoreDownTime = time.Minute
// If a store's last heartbeat is before now-storeDisconnectDuration, the store

This comment has been minimized.

@huachaohuang

huachaohuang Sep 29, 2017

Member

It's hard to read, may be:
"If a store's last heartbeat is storeDisconnectDuration ago, ...."

@huachaohuang

huachaohuang Sep 29, 2017

Member

It's hard to read, may be:
"If a store's last heartbeat is storeDisconnectDuration ago, ...."

const storeSpaceThreshold = 0.2
// LowSpace checks if the store is lack of space.
func (s *StoreInfo) LowSpace() bool {

This comment has been minimized.

@huachaohuang

huachaohuang Sep 29, 2017

Member

I think IsLowSpace is better.

@huachaohuang

huachaohuang Sep 29, 2017

Member

I think IsLowSpace is better.

@@ -133,6 +133,13 @@ func (s *StoreInfo) AvailableRatio() float64 {
return float64(s.Stats.GetAvailable()) / float64(s.Stats.GetCapacity())
}
const storeSpaceThreshold = 0.2

This comment has been minimized.

@huachaohuang

huachaohuang Sep 29, 2017

Member

I think storeLowSpaceThreshold is better.

@huachaohuang

huachaohuang Sep 29, 2017

Member

I think storeLowSpaceThreshold is better.

func newStoreInfo(store *core.StoreInfo) *storeInfo {
func newStoreInfo(store *core.StoreInfo, maxStoreDownTime time.Duration) *storeInfo {

This comment has been minimized.

@huachaohuang

huachaohuang Sep 29, 2017

Member

Since we pass the maxStoreDownTime to storeInfo now, we can provide a IsDown method for convinience.

@huachaohuang

huachaohuang Sep 29, 2017

Member

Since we pass the maxStoreDownTime to storeInfo now, we can provide a IsDown method for convinience.

This comment has been minimized.

@nolouch

nolouch Sep 29, 2017

Member

This struct just for the api show. no other place use it. I think it is ok.

@nolouch

nolouch Sep 29, 2017

Member

This struct just for the api show. no other place use it. I think it is ok.

This comment has been minimized.

@huachaohuang

huachaohuang Sep 29, 2017

Member

I see.

@huachaohuang
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment