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

Stop thread pool on shutdown #2311

Merged
merged 10 commits into from Sep 27, 2017

Conversation

Projects
None yet
6 participants
@huachaohuang
Member

huachaohuang commented Sep 18, 2017

Shutdown without stopping thread pool may cause potentially
core dump. However, stopping thread pool before shutdown may slow down
the shutdown process, unless we can cancel the running task, which
seems not easy to do.

Stop thread pool on shutdown
Shutdown without stopping thread pool may cause potentially
coredump. However, stopping thread pool before shutdown may slow down
the shutdown process, unless we can cancel the running task, which
seems not easy to do.
@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Sep 18, 2017

Contributor

LGTM

Contributor

siddontang commented Sep 18, 2017

LGTM

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang
Contributor

siddontang commented Sep 19, 2017

@overvenus

This comment has been minimized.

Show comment
Hide comment
@overvenus

overvenus Sep 19, 2017

Member

Please also stop snap-worker thread pool.
https://github.com/pingcap/tikv/blob/fbf6e28a7418eb2d3c2ac3490276141cccf38115/src/server/snap.rs#L182

Maybe Runable should not provide a default shutdown()

Member

overvenus commented Sep 19, 2017

Please also stop snap-worker thread pool.
https://github.com/pingcap/tikv/blob/fbf6e28a7418eb2d3c2ac3490276141cccf38115/src/server/snap.rs#L182

Maybe Runable should not provide a default shutdown()

@BusyJay

This comment has been minimized.

Show comment
Hide comment
@BusyJay

BusyJay Sep 19, 2017

Contributor

According to experience, TiKV may wait for serveral hours for snap-worker to stop.

Contributor

BusyJay commented Sep 19, 2017

According to experience, TiKV may wait for serveral hours for snap-worker to stop.

@BusyJay

This comment has been minimized.

Show comment
Hide comment
@BusyJay

BusyJay Sep 19, 2017

Contributor

May be exit(0) in TiKV server is a clean way to stop without wait for all threads to stop, which is unnecessary and possibly time consuming.

Contributor

BusyJay commented Sep 19, 2017

May be exit(0) in TiKV server is a clean way to stop without wait for all threads to stop, which is unnecessary and possibly time consuming.

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Sep 20, 2017

Contributor

IMO, I prefer closing gracefully.

TiKV may wait for serveral hours for snap-worker to stop

@BusyJay Why this takes too much time? Can we quit ASAP?

Contributor

siddontang commented Sep 20, 2017

IMO, I prefer closing gracefully.

TiKV may wait for serveral hours for snap-worker to stop

@BusyJay Why this takes too much time? Can we quit ASAP?

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Sep 20, 2017

Contributor

PTAL

Contributor

siddontang commented Sep 20, 2017

PTAL

@BusyJay

This comment has been minimized.

Show comment
Hide comment
@BusyJay

BusyJay Sep 20, 2017

Contributor

It depends. If io is slow, then write can be slow, sometimes it may even stall.

Contributor

BusyJay commented Sep 20, 2017

It depends. If io is slow, then write can be slow, sometimes it may even stall.

@@ -303,4 +303,10 @@ impl Runnable<Task> for Runner {
} => self.ctx.handle_destroy(region_id, start_key, end_key),
}
}
fn shutdown(&mut self) {

This comment has been minimized.

@siddontang

siddontang Sep 20, 2017

Contributor

seem we don't use this in this PR.

@siddontang

siddontang Sep 20, 2017

Contributor

seem we don't use this in this PR.

This comment has been minimized.

@huachaohuang

huachaohuang Sep 20, 2017

Member

It will be called outside automatically.

@huachaohuang

huachaohuang Sep 20, 2017

Member

It will be called outside automatically.

This comment has been minimized.

@zhangjinpeng1987

zhangjinpeng1987 Sep 21, 2017

Member

This thread may need a long time to finish?

@zhangjinpeng1987

zhangjinpeng1987 Sep 21, 2017

Member

This thread may need a long time to finish?

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Sep 20, 2017

Contributor

I think we can consider stopping snap-worker later.

Contributor

siddontang commented Sep 20, 2017

I think we can consider stopping snap-worker later.

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Sep 21, 2017

Contributor

PTAL

Contributor

siddontang commented Sep 21, 2017

PTAL

@overvenus

This comment has been minimized.

Show comment
Hide comment
@overvenus

overvenus Sep 22, 2017

Member

/run-all-test

Member

overvenus commented Sep 22, 2017

/run-all-test

@siddontang

This comment has been minimized.

Show comment
Hide comment
@siddontang

siddontang Sep 23, 2017

Contributor

CI failed @huachaohuang

Contributor

siddontang commented Sep 23, 2017

CI failed @huachaohuang

@nolouch nolouch merged commit 5f2a1b2 into master Sep 27, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tikv/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@nolouch nolouch deleted the huachao/shutdown branch Sep 27, 2017

@huachaohuang

This comment has been minimized.

Show comment
Hide comment
@huachaohuang

huachaohuang Sep 27, 2017

Member

Uh, we may need to figure out a better way to stop long time task on shutdown.

Member

huachaohuang commented Sep 27, 2017

Uh, we may need to figure out a better way to stop long time task on shutdown.

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