-
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
Support leader change in meta client && do some refactors #354
Conversation
Unit testing passed. |
src/meta/client/MetaClient.cpp
Outdated
remoteFunc(client, std::move(req)) | ||
.then(evb, [p = std::move(pro), respGen, this] (folly::Try<RpcResponse>&& t) mutable { | ||
// exception occurred during RPC | ||
if (t.hasException()) { | ||
p.setValue(Status::Error(folly::stringPrintf("RPC failure in MetaClient: %s", | ||
t.exception().what().c_str()))); | ||
{ | ||
folly::RWSpinLock::WriteHolder holder(hostLock_); |
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.
Why not put hostLock_ inside updateHost()?
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.
Sounds reasonable
@@ -35,7 +35,7 @@ ServerBasedSchemaManager::getTagSchema(GraphSpaceID space, TagID tag, SchemaVer | |||
if (ver < 0) { | |||
ver = getNewestTagSchemaVer(space, tag); | |||
} | |||
auto ret = metaClient_->getTagSchemeFromCache(space, tag, ver); | |||
auto ret = metaClient_->getTagSchemaFromCache(space, tag, ver); |
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.
Bright eyes
return Status::Error("existed!"); | ||
case cpp2::ErrorCode::E_NOT_FOUND: | ||
return Status::Error("not existed!"); | ||
case cpp2::ErrorCode::E_LEADER_CHANGED: { |
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.
When the leader changes ,getResponse() returns the error code E_LEADER_CHANGED, then the resend message is triggered by the trigger itself(Sometimes is users, but they don't need know this).
Why not try to resend through getResponse() or unified process?
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.
Retry logic is another story not related to this pr.
Unit testing passed. |
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.
LGTM
Unit testing passed. |
…#354) * Support leader change in meta client && do some refactors * Address @laura-ding's comments * Fix remove tag schema bug
Co-authored-by: kyle.cao <[email protected]>
subtask of #283