-
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
Integrate raft into storage #405
Conversation
jenkins go |
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.
The pr looks better now. See my inline comments.
jenkins go |
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.
Now, the pr looks good to me now. Let's merge it firstly before #421
@@ -83,19 +83,29 @@ StatusOr<std::unordered_map<std::string, std::string>> NetworkUtils::listDeviceA | |||
return dev2ipv4s; | |||
} | |||
|
|||
|
|||
bool NetworkUtils::getDynamicPortRange(uint16_t& low, uint16_t& high) { |
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.
Does return value use StatusOr?
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 quite understand why we need StatusOr here
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.
Need changes.
All UTs pass. At the moment, all parts still running with single copy. Will enable multiple copies in the future PR Implemented #178
Rebased as well
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
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.
Let's end the long journey.
cb(ret, HostAddr(0, 0)); | ||
|
||
wal::BufferFlusher* getBufferFlusher() { | ||
static wal::BufferFlusher flusher; |
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.
Only one flusher maybe a bottleneck. Let's open another issue to make the number configurable.
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 could make the number of flushing threads in the BufferFlusher configurable
Sure. let's do it in a separate PR
Unit testing passed. |
Unit testing passed. |
* Integrate raft into storage All UTs pass. At the moment, all parts still running with single copy. Will enable multiple copies in the future PR Implemented vesoft-inc#178 * Addressed @dangleptr's comments and rebased * Addressed @dangleptr's comments and rebased * Addressed @laura-ding's @dangleptr's comments Rebased as well
* Integrate raft into storage All UTs pass. At the moment, all parts still running with single copy. Will enable multiple copies in the future PR Implemented vesoft-inc#178 * Addressed @dangleptr's comments and rebased * Addressed @dangleptr's comments and rebased * Addressed @laura-ding's @dangleptr's comments Rebased as well
Force push so that nebula dev could switch to third-party 3.0 Co-authored-by: Sherman <[email protected]>
All UTs pass. At the moment, all parts still running with single copy. Will enable multiple copies in the future PR
Implemented #178