-
Notifications
You must be signed in to change notification settings - Fork 85
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
Dynamic backends #705
base: master
Are you sure you want to change the base?
Dynamic backends #705
Conversation
|
@@ -154,6 +159,7 @@ class config_data | |||
config_data &operator() (const std::string &name, const variant &value); | |||
const variant *value_impl(const std::string &name) const; | |||
|
|||
bool m_serializable; |
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.
Please, add description to m_serializable
and its getter/setter.
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.
done
return std::shared_ptr<dnet_backend_info>(); | ||
} | ||
|
||
void dnet_backend_info_manager::add_backend(std::shared_ptr<dnet_backend_info> &backend) |
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.
Are you sure this does not leak one reference?
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.
Yep
Please elaborate, what does this huge patch do? Why is this needed, what was the problem, why have you decided to 'fix' it this way? |
backends array of dnet_backend_io structs in dnet_io was changed to array of pointers. It allows quick reallocating of the array for adding new backends on the fly, but introduces locking mechanics for every access to backends array via rw-lock. For the same reasons backends vector in dnet_config_data was changed to a hash table encapsulated in dnet_backend_info_manager class. |
There is definitely no quick reallocation, since you copy the whole array again and again when adding new elements. Let's start with more fundamental questions: What is the purpose of this patch? What is the problem it tries to solve? Is it present in the list of issues to solve first which was introduced like a year ago? |
This patch is the development of #661. The issue behind these changes is: allow users to add/remove backends in dynamic without restarting dnet_ioserv. These patch introduces minimal changes to solve the issue. The next step is refactoring |
Reallocation of small array of pointers is a pretty fast operation. |
return 0; | ||
} | ||
|
||
struct dnet_backend_io *dnet_get_backend_io(struct dnet_io *io, size_t backend_id) |
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.
This function doesn't increase reference counter or perform any other control flow operations (like remove object from array) else, why does it use locking?
Server parses config and starts backend, which was not presented in config initially, on DNET_BACKEND_ENABLE command. Thus, it is possible to add backends on the fly.