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

librbd: Notifier::notify API improvement #14072

Merged
merged 2 commits into from Apr 4, 2017

Conversation

Projects
None yet
2 participants
@trociny
Contributor

trociny commented Mar 21, 2017

Replace the out bufferlist with a response struct.

Signed-off-by: Mykola Golub mgolub@mirantis.com

@trociny

This comment has been minimized.

Contributor

trociny commented Mar 21, 2017

@dillaman In WatchNotifyTypes.h we have watch_notify::ClientId, similar to the newly added watcher::Client. Do you think it makes sense to move ClientId to watcher/Types.h instead of adding Client there?

@dillaman

This comment has been minimized.

Contributor

dillaman commented Mar 21, 2017

@trociny Sure -- sounds good to me.

@dillaman dillaman self-requested a review Mar 21, 2017

@trociny

This comment has been minimized.

Contributor

trociny commented Mar 21, 2017

@dillaman
My attempt to move ClientId from WatchNotifyTypes.h to watcher/Types.h failed, because watcher/Types.cc is not included to rbd_types lib, and dencoder fails to build. And I can't include watcher/Types.cc to rbd_types lib, because it depends on Watcher.

So, now I am just using watch_notify::ClientId in watcher::NotifyResponse.

The other issue, is that the compiler failed to resolve ::encode(acks, bl) (and so) until I added WRITE_CLASS_ENCODER(ClientId) to WatchNotifyTypes.h (the definition WRITE_CLASS_ENCODER(librbd::watch_notify::ClientId) out of librbd::watch_notify namespace does not work by some reason).

Context *on_finish;
bufferlist *out_bl;

This comment has been minimized.

@dillaman

dillaman Mar 27, 2017

Contributor

Nit: any reason why we can't just inline the bufferlist instead of conditionally allocating the buffer if a NotifyResponse was provided? This isn't a high-profile path so for the cleaner code, I think we can handle the potentially wasted memory allocation.

@dillaman

One possible solution for your ClientId issue is to rename the current "watcher/Types.cc/h" files to "watcher/Utils.cc/h" (w/ the surrounding namespace) since they really don't follow the style and usage pattern of structs defined in "Types.h" files in other areas of librbd.

@dillaman

This comment has been minimized.

Contributor

dillaman commented Mar 27, 2017

... also, C_NotifyAck could move directly into Watcher since it only ever appears to be used by classes directly inheriting from Watcher (to avoid having to put an additional namespace in all the existing payload functions).

@trociny

This comment has been minimized.

Contributor

trociny commented Mar 28, 2017

@dillaman updated

@@ -17,9 +17,20 @@ class ContextWQ;
namespace librbd {
class Watcher {
friend struct watcher::C_NotifyAck;
friend struct C_NotifyAck;

This comment has been minimized.

@dillaman

dillaman Mar 28, 2017

Contributor

Nit: no longer required

@@ -0,0 +1,14 @@
// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-

This comment has been minimized.

@dillaman

dillaman Mar 28, 2017

Contributor

Nit: doesn't look like this file is needed (i.e. no need for the explicit template instantiation below).

Mykola Golub added some commits Mar 21, 2017

Mykola Golub
librbd: Notifier::notify API improvement
Replace the out bufferlist with a response struct.

Signed-off-by: Mykola Golub <mgolub@mirantis.com>
Mykola Golub
librbd: refactor watcher types
Signed-off-by: Mykola Golub <mgolub@mirantis.com>
@trociny

This comment has been minimized.

Contributor

trociny commented Mar 28, 2017

@dillaman updated

@dillaman

lgtm

@dillaman

This comment has been minimized.

Contributor

dillaman commented Apr 4, 2017

retest this please

@dillaman dillaman merged commit b1f1df4 into ceph:master Apr 4, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment