Set of functionality additions and small adjustments. #56
Conversation
|
This PR primarily has functionality addtions for the librados integration plus a few other related changes. |
ivancich
force-pushed
the
wip-librados-adjustments
branch
2 times, most recently
from
abacfd0 to
c2a1e93
Compare
src/dmclock_server.h
Outdated
| } | ||
|
|
||
|
|
||
| ~PriorityQueueBase() { | ||
| finishing = true; | ||
|
|
||
| DataGuard g(data_mtx); |
There was a problem hiding this comment.
locking in a destructor is generally unnecessary, unless it's to synchronize with the shutdown of an internal thread. you don't need to provide mutual exclusion between the destructor and public functions, because it would already be undefined behavior for one thread to call functions on an object that's being destructed in another thread
src/dmclock_server.h
Outdated
| for (auto c = client_map.begin(); client_map.end() != c; /* empty */) { | ||
| auto current = c++; | ||
| for (auto l : idle_erase_listeners) { | ||
| (*l)(current->second->client); |
There was a problem hiding this comment.
what's the motivation for notifying these listeners on destruction?
There was a problem hiding this comment.
The key motivation was that if they were maintaining data to support those clients (i.e., they were the target of a ClientInfoFunc) they could dispose of the data. This was specifically for the OSD. However in the case of the OSD it's unlikely the dmclock server code (i.e., PullPriorityQueue) would be destructed if the rest of the OSD weren't also destructed. But in a more general case it seems necessary to prevent irreversible memory growth.
There was a problem hiding this comment.
As I think about this further, not allowing the listener to be dynamic constrains the owner but likely makes more sense overall.
src/dmclock_server.h
Outdated
|
|
||
| // NB: All threads declared at end, so they're destructed first! | ||
| using IdleEraseListener = std::function<void(const C&)>; | ||
| std::set<const IdleEraseListener*> idle_erase_listeners; |
There was a problem hiding this comment.
it doesn't seem safe to store these functions by pointer - especially when they're passed to add_idle_erase_listener() as a const reference. that means it's perfectly valid to call with a lambda, which we'd store as a pointer to a temporary object:
q.add_idle_erase_listener([] (const C& c) {});it looks like it was done this way so you could implement remove_idle_erase_listener() - but is this kind of dynamic registration really necessary? consider treating this similar to ClientInfoFunc, and take a single optional callback as a constructor argument. if the caller really needs dynamic registration, they could implement it themselves inside their callback. does that make sense?
ivancich
force-pushed
the
wip-librados-adjustments
branch
2 times, most recently
from
c27cdbf to
6ce61d0
Compare
|
@cbodley Thanks for all your input. I reworked it and I think the code is much better now. |
ivancich
force-pushed
the
wip-librados-adjustments
branch
from
6ce61d0 to
ddd6b56
Compare
First it allows a ClientProfile to be updated with new reservation, weight, and limit values by adding an update function. Second it adds an ability to invoke callbacks when a ClientInfo object is removed due to the idle timeout. Testing this functionality has been added to the unit tests. Third we add the ability to clear a heap to help with a more controlled tear-down. Finally, dmclock-servers "cleaning" has been renamed "idle erase" to better indicate the role. Types and variable names have been adjusted accordingly. Signed-off-by: J. Eric Ivancich <[email protected]>
ivancich
force-pushed
the
wip-librados-adjustments
branch
from
ddd6b56 to
d76b9af
Compare
|
@ivancich i will review this change later this week. but do you mind splitting this change into smaller commits? i can help on this with your permission if you don't have enough bandwidth on this. |
This commit contains a bunch of minor changes.
First it allows a ClientProfile to be updated with new reservation,
weight, and limit values by adding an update function.
Second it adds an ability to invoke callbacks when a ClientInfo object
is removed due to the idle timeout. Testing this functionality has
been added to the unit tests.
Third we add the ability to clear a heap to help with a more
controlled tear-down.
Finally, dmclock-servers "cleaning" has been renamed "idle erase" to
better indicate the role. Types and variable names have been adjusted
accordingly.
Signed-off-by: J. Eric Ivancich [email protected]