-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fixed a bug where the storage service might crash (#583) #595
Fixed a bug where the storage service might crash (#583) #595
Conversation
summary: When the storage service deamon is started, the nebula::meta::MetaClient will use the MetaServerBasedPartManager object(MetaClient::registerListener), but the MetaServerBasedPartManager object will be released before nebula::meta::MetaClient(through KVStore object). Added the MetaClient::unRegisterListener method to stop the background thread and reset the listener_ object, And the unRegisterListener is called when the MetaServerBasedPartManager object was released.
Unit testing passed. |
I am a little confused. Since PartManager |
That's because two threads will be started in metaclient, and MetaServerBasedPartManager will be used in these two threads. |
src/meta/client/MetaClient.h
Outdated
listener_ = listener; | ||
} | ||
|
||
void unRegisterListener() { | ||
deInit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood now. Maybe you should not call deInit here? Because other's may still use the bgThread in MetaClient, e.g. SchemaManager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we need to stop the background thread first, otherwise it will have race conditions in listener_.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd better protect listener_ in MetaClient instead of stop bgThread directly. Because we don't want meta client is controlled by kvstore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to my understanding.
1 metaclient itself is dependent on the listener_ object.
2 If listener_ is released, both two threads of metaclient will not work, then there is no need for metaclient at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly. Listener_ is not necessary for meta client.
Use a read-write lock in metaclient to protect the listener_.
Unit testing passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally, the pr lgtm. Does the crash problem fixed after unRegisterListener?
src/meta/client/MetaClient.h
Outdated
@@ -69,9 +69,16 @@ class MetaClient { | |||
void init(); | |||
|
|||
void registerListener(MetaChangedListener* listener) { | |||
CHECK(listener_ == nullptr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the check under the lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally, the pr lgtm. Does the crash problem fixed after unRegisterListener?
Yes, this PR will fix the crash.
Fixed possible thread competition for checking listener_ .
Unit testing passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done. LGTM. Thanks for taking care of it.
Unit testing passed. |
…esoft-inc#595) * Fixed a bug where the storage service might crash (vesoft-inc#583) summary: When the storage service deamon is started, the nebula::meta::MetaClient will use the MetaServerBasedPartManager object(MetaClient::registerListener), but the MetaServerBasedPartManager object will be released before nebula::meta::MetaClient(through KVStore object). Added the MetaClient::unRegisterListener method to stop the background thread and reset the listener_ object, And the unRegisterListener is called when the MetaServerBasedPartManager object was released. * Fixed a bug where the storage service might crash (vesoft-inc#583) Use a read-write lock in metaclient to protect the listener_. * Fixed a bug where the storage service might crash (vesoft-inc#583) Fixed possible thread competition for checking listener_ .
* uuid * uuid Co-authored-by: jakevin <30525741+jackwener@users.noreply.github.com>
summary:
When the storage service deamon is started, the nebula::meta::MetaClient
will use the MetaServerBasedPartManager object(MetaClient::registerListener),
but the MetaServerBasedPartManager object will be released before
nebula::meta::MetaClient(through KVStore object).
Added the MetaClient::unRegisterListener method to stop the background thread
and reset the listener_ object, And the unRegisterListener is called
when the MetaServerBasedPartManager object was released.