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.

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

*: ignore tombstone stores #4223

Merged
merged 14 commits into from Feb 26, 2019

Conversation

Projects
None yet
8 participants
@overvenus
Copy link
Member

overvenus commented Feb 15, 2019

What have you changed? (mandatory)

Ignore tombstone stores.

What are the type of the changes? (mandatory)

  • Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

Unit tests.

Does this PR affect documentation (docs) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No.

Refer to a related PR or issue link (optional)

Fixes #4221
Cc pingcap/tidb#9269

*: ignore tombstone stores
Signed-off-by: Neil Shen <overvenus@gmail.com>

@overvenus overvenus force-pushed the overvenus:ignore-tombstone branch from 8bfbdb7 to b682c27 Feb 15, 2019

@overvenus overvenus added the S: WIP label Feb 15, 2019

pd: return an error when get a tombstone store
Signed-off-by: Neil Shen <overvenus@gmail.com>

@overvenus overvenus removed the S: WIP label Feb 15, 2019

if store.get_state() != metapb::StoreState::Tombstone {
Ok(store)
} else {
Err(Error::StoreTombstone(format!("{:?}", store)))

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Feb 15, 2019

Member

Why return error when the store is Tombstone?

This comment has been minimized.

Copy link
@overvenus

overvenus Feb 15, 2019

Author Member

Tombstone stores are no longer belongs to the cluster, other TiKVs should ignore such stores. See more pingcap/tidb#9269

This comment has been minimized.

Copy link
@siddontang

siddontang Feb 18, 2019

Contributor

seem we don't need to use a string for the StoreTombstone. maybe integer is enough.

This comment has been minimized.

Copy link
@overvenus

overvenus Feb 18, 2019

Author Member

PD may return StoreTombston error too, but it does not contain a store id, so I choose string for simplicity.

let err = header.get_error();
match err.get_field_type() {
ErrorType::ALREADY_BOOTSTRAPPED => Err(Error::ClusterBootstrapped(header.get_cluster_id())),
ErrorType::NOT_BOOTSTRAPPED => Err(Error::ClusterNotBootstrapped(header.get_cluster_id())),
ErrorType::INCOMPATIBLE_VERSION => Err(Error::Incompatible),
_ => Err(box_err!(err.get_message())),
ErrorType::STORE_TOMBSTONE => Err(Error::StoreTombstone(err.get_message().to_owned())),
ErrorType::UNKNOWN => Err(box_err!(err.get_message())),

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Feb 15, 2019

Member

When the UNKNOWN error type will be returned?

This comment has been minimized.

Copy link
@overvenus

overvenus Feb 15, 2019

Author Member

It depends on PD.

@overvenus overvenus added the T: BugFix label Feb 15, 2019

Show resolved Hide resolved src/pd/client.rs Outdated
Show resolved Hide resolved src/pd/client.rs Outdated
@cwen0

This comment has been minimized.

Copy link
Contributor

cwen0 commented Feb 16, 2019

/test

}

fn get_all_stores(&self) -> Result<Vec<metapb::Store>> {
fn get_all_stores(&self, include_tombstone: bool) -> Result<Vec<metapb::Store>> {

This comment has been minimized.

Copy link
@zhangjinpeng1987

zhangjinpeng1987 Feb 16, 2019

Member

I think PD support such API is more elegant. BTW, how lightning interact with this API?

This comment has been minimized.

Copy link
@kennytm

kennytm Feb 16, 2019

Contributor

Lightning doesn't interact with TiKV stores directly, Importer does (src/import/import.rs in this PR).

Importer uses this API to get all TiKV nodes to forward the "switch to import mode" or "compact cluster" command. It doesn't affect SST ingestion.

This comment has been minimized.

Copy link
@zhangjinpeng1987

This comment has been minimized.

Copy link
@overvenus

overvenus Feb 18, 2019

Author Member

Cc @nolouch What do you think? Is it ok to support this feature in PD side?

This comment has been minimized.

Copy link
@nolouch

nolouch Feb 19, 2019

Contributor

I think it's ok to do that.

overvenus added some commits Feb 18, 2019

Address comments
Signed-off-by: Neil Shen <overvenus@gmail.com>
@zhangjinpeng1987
Copy link
Member

zhangjinpeng1987 left a comment

LGTM

Show resolved Hide resolved src/import/client.rs Outdated

overvenus added some commits Feb 21, 2019

Address comments
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Feb 21, 2019

/run-all-tests

@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Feb 22, 2019

PTAL
/run-all-tests

@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Feb 22, 2019

@huachaohuang PTAL, thanks!

@huachaohuang

This comment has been minimized.

Copy link
Contributor

huachaohuang commented Feb 22, 2019

When I first added this get_all_stores API, I didn't expect PD to return tombstone stores at all (yes, I implemented that API in PD too, sorry 😂). IMO, it's more elegant to fix this in PD, and we can add an option like include_tombstones in the request if we really need that in the future.

@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Feb 22, 2019

IMO, it's more elegant to fix this in PD, and we can add an option like include_tombstones in the request if we really need that in the future.

Yes, but it does not with to this PR, they all have the same API. Some users already met this bug, so I think we should merge this PR ASAP.

@overvenus overvenus requested a review from huachaohuang Feb 22, 2019

@huachaohuang

This comment has been minimized.

Copy link
Contributor

huachaohuang commented Feb 22, 2019

@overvenus how about just fixing this in PD?
pingcap/pd#1440

@kennytm

This comment has been minimized.

Copy link
Contributor

kennytm commented Feb 22, 2019

Could we have a solution that can be easily cherry-picked to 2.1?

use exclude tombstone stores
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Feb 26, 2019

PTAL, thanks!

@zhangjinpeng1987
Copy link
Member

zhangjinpeng1987 left a comment

LGTM

@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Feb 26, 2019

/run-all-tests

@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Feb 26, 2019

/run-all-tests

@overvenus

This comment has been minimized.

Copy link
Member Author

overvenus commented Feb 26, 2019

/rebuild

@overvenus overvenus merged commit 9a66560 into tikv:master Feb 26, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@overvenus overvenus deleted the overvenus:ignore-tombstone branch Feb 26, 2019

overvenus added a commit to overvenus/tikv that referenced this pull request Feb 26, 2019

*: ignore tombstone stores (tikv#4223)
Signed-off-by: Neil Shen <overvenus@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.