Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

Meta handles leader changes and other errorcodes uniformly #423

Merged
merged 1 commit into from
Apr 19, 2021
Merged

Meta handles leader changes and other errorcodes uniformly #423

merged 1 commit into from
Apr 19, 2021

Conversation

panda-sheep
Copy link
Contributor

@panda-sheep panda-sheep commented Apr 6, 2021 •
edited
Loading

For meta, when the leader changes, it is not handled correctly in most cases, and an incorrect error code is returned. So the leader of the meta was changed, but the meta client received other error codes and did not try again.

This pr uniformly handles the error codes of various operations in meta BaseProcessor and so on.
benefit:

  1. The leader change situation of meta can be processed uniformly .
  2. It also facilitates the unified processing of subsequent errorcodes.

@panda-sheep panda-sheep added the wip Solution: work in progress label Apr 6, 2021
@critical27
Copy link
Contributor

Good, just remove all StatusOr in meta...

@panda-sheep panda-sheep mentioned this pull request Apr 7, 2021
Merged
@panda-sheep panda-sheep force-pushed the handle_leader_changes_and_other_errorcodes_uniformly branch 5 times, most recently from 5d990d0 to 3b8e06d Compare April 9, 2021 03:15
@panda-sheep panda-sheep added ready-for-testing PR: ready for the CI test Ready-to-review and removed wip Solution: work in progress labels Apr 9, 2021
@panda-sheep panda-sheep force-pushed the handle_leader_changes_and_other_errorcodes_uniformly branch from 14479d6 to 0987a28 Compare April 9, 2021 08:41
critical27
critical27 reviewed Apr 12, 2021
View reviewed changes
Copy link
Contributor

@critical27 critical27 left a comment

Choose a reason for hiding this comment

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

Great work, so many places.

src/meta/MetaServiceUtils.cpp Show resolved Hide resolved
src/meta/common/MetaCommon.h Show resolved Hide resolved
@panda-sheep panda-sheep force-pushed the handle_leader_changes_and_other_errorcodes_uniformly branch from 0987a28 to 058a09a Compare April 13, 2021 10:13
@bright-starry-sky
Copy link
Contributor

nice job，massive project.

bright-starry-sky
bright-starry-sky reviewed Apr 14, 2021
View reviewed changes
src/meta/ActiveHostsMan.cpp
@@ -35,20 +36,22 @@ kvstore::ResultCode ActiveHostsMan::updateHostInfo(kvstore::KVStore* kv,
baton.post();
});
baton.wait();
return ret;
return MetaCommon::to(ret);
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought, should we completely unified kvstore::ResultCode and cpp2::ErrorCode ?
For historical reasons, kvstore module cannot depend on storage module, but now that doesn't seem to be the case. so can we using the cpp2::ErrorCode in kvstore module direct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strongly Agree, 👍
At the storage, these two errorcodes are too annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this pr is handled like this first, and then I will use a new pr to handle it, otherwise this pr is too big. wdyt?

@panda-sheep panda-sheep force-pushed the handle_leader_changes_and_other_errorcodes_uniformly branch 6 times, most recently from 0a0a13b to 544935a Compare April 15, 2021 10:46
@critical27
Copy link
Contributor

Conflict again, maybe because there are several reverts these days. You could contact @liuyu85cn if necessary.

@panda-sheep panda-sheep force-pushed the handle_leader_changes_and_other_errorcodes_uniformly branch from 544935a to 778f5f7 Compare April 15, 2021 12:38
@panda-sheep panda-sheep force-pushed the handle_leader_changes_and_other_errorcodes_uniformly branch 2 times, most recently from 94ae89f to 878349b Compare April 15, 2021 16:17
@panda-sheep panda-sheep force-pushed the handle_leader_changes_and_other_errorcodes_uniformly branch from 878349b to a52cc2a Compare April 16, 2021 08:27
@panda-sheep panda-sheep mentioned this pull request Apr 16, 2021
Merged
@panda-sheep
Copy link
Contributor Author

test passed, ready to review.

critical27
critical27 approved these changes Apr 19, 2021
View reviewed changes
Copy link
Contributor

@critical27 critical27 left a comment

Choose a reason for hiding this comment

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

Good job. LGTM

liuyu85cn
liuyu85cn approved these changes Apr 19, 2021
View reviewed changes
Copy link
Contributor

@liuyu85cn liuyu85cn left a comment

Choose a reason for hiding this comment

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

LGTM

@critical27 critical27 merged commit 9bfb3e6 into vesoft-inc:master Apr 19, 2021
@panda-sheep panda-sheep deleted the handle_leader_changes_and_other_errorcodes_uniformly branch April 19, 2021 03:30
@panda-sheep panda-sheep mentioned this pull request Apr 19, 2021
Merged
@laura-ding laura-ding mentioned this pull request Jun 15, 2021
Closed
3 tasks
@wey-gu wey-gu mentioned this pull request Aug 30, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bright-starry-sky bright-starry-sky bright-starry-sky left review comments

@darionyaphet darionyaphet darionyaphet left review comments

@liuyu85cn liuyu85cn liuyu85cn approved these changes

@critical27 critical27 critical27 approved these changes

@monadbobo monadbobo Awaiting requested review from monadbobo

Assignees
No one assigned
Labels
ready-for-testing PR: ready for the CI test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@panda-sheep @critical27 @bright-starry-sky @darionyaphet @liuyu85cn