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

*: implements health check #518

Merged
merged 9 commits into from
Mar 16, 2021
Merged

*: implements health check #518

merged 9 commits into from
Mar 16, 2021

Conversation

BusyJay
Copy link
Member

@BusyJay BusyJay commented Mar 12, 2021

This PR provides simple health check implementations referring to the go
version. It provides a HealthService to maintain the service statuses
and wake up watchers.

I use a standalone crate to avoid introduce grpc specific code into the grpcio
crate. And I don't reuse grpcio-proto to avoid the dependency of protobuf-build
which just make things complicated by introducing a lot of dependencies and
resulting in a dependency circle.

This PR provides simple health check implementations referring to the go
version. It provides a `HealthService` to maintain the service statuses
and wake up watchers.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay
Copy link
Member Author

BusyJay commented Mar 12, 2021

Most code are generated by protobuf and prost, you can just skip them as long as it compiles.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
This reverts commit c320d22.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
/// Updates the status to specified one and update version.
fn broadcast(&self, status: ServingStatus) {
let mut subscribers = self.subscribers.lock().unwrap();
let state = self.state.load(Ordering::Acquire);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guarded by the subscribers mutex, I think it can be Relaxed.


let mut subscribers = s.cast.subscribers.lock().unwrap();

let cur_state = s.cast.state.load(Ordering::Acquire);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be Relaxed because a mutex has memory fence itself.

let mut subscribers = self.subscribers.lock().unwrap();
let state = self.state.load(Ordering::Acquire);
let new_state = ((state + VERSION_STEP) & !STATUS_MASK) | (status as usize);
self.state.store(new_state, Ordering::Release);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And because L74 loads with Relaxed, this can be Relaxed as well.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay
Copy link
Member Author

BusyJay commented Mar 15, 2021

I add back protobuf-build to make it work well with TiKV. TiKV seems to use custom protobuf patch which doesn't work well with latest published one.

Code generated by protobuf-build in TiKV will change struct's name, so it's impossible to make them compatible with each other when generate code dynamically. Instead, I use code generated by an old version of protobuf and inline them.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
sticnarf
sticnarf previously approved these changes Mar 16, 2021
Copy link
Contributor

@sticnarf sticnarf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rest LGTM

@@ -0,0 +1,161 @@
// Copyright 2019 TiKV Project Authors. Licensed under Apache-2.0.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Copyright 2019 TiKV Project Authors. Licensed under Apache-2.0.
// Copyright 2021 TiKV Project Authors. Licensed under Apache-2.0.


let mut subscribers = s.cast.subscribers.lock().unwrap();

let cur_state = s.cast.state.load(Ordering::Relaxed);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why check it twice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first check is a fast path. Only section protected by lock can guarantee state is not updated by others.

impl StatusCast {
fn new(status: ServingStatus) -> StatusCast {
StatusCast {
state: AtomicUsize::new(VERSION_STEP | (status as usize)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we can't use status directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you can't solve the ABA problem.

Connor1996
Connor1996 previously approved these changes Mar 16, 2021
Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
…impl-health-server

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>
@BusyJay BusyJay dismissed stale reviews from Connor1996 and sticnarf via 1955c6c March 16, 2021 07:34
@BusyJay BusyJay merged commit f897d1d into tikv:master Mar 16, 2021
@BusyJay BusyJay deleted the impl-health-server branch March 16, 2021 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants