Skip to content
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.

Sign up for GitHub

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

Jump to bottom

common: mark events of TrackedOp outside its constructor #19828

Merged
merged 1 commit into from Jan 26, 2018
Merged

common: mark events of TrackedOp outside its constructor #19828

merged 1 commit into from Jan 26, 2018

Conversation

xxhdx1985126
Copy link
Contributor

@xxhdx1985126 xxhdx1985126 commented Jan 8, 2018 •
edited

Avoid marking events when TrackedOp::tracking_start() isn't invoked yet.

Fixes: http://tracker.ceph.com/issues/22608
Signed-off-by: Xuehan Xu xuxuehan@360.cn

@gregsfortytwo
Copy link
Member

Hmm, do we know why tracking_start is invoked where it is? That function isn’t familiar to me but it smells like performance.

@xxhdx1985126
Copy link
Contributor Author

xxhdx1985126 commented Jan 9, 2018 •
edited

@gregsfortytwo Hi, thanks for the review:-)
As far as I understand, tracking_start is to enable tracking of a TrackedOp if OpTracker::tracking_enabled is true, after which various events can be marked for the TrackedOp. However, in the case of OpRequest, initial events like "header_read" are marked in its constructor while tracking_start is invoked after the construction of a TrackedOp, which means events like "header_read" is not marked, although we intend to mark it, as tracking is not enabled yet. So I move the calling of tracking_start to the constructor of TrackedOp, which would make sure that all events marking operations happen after the tracking_start. Is this the right way to go? Thanks:-)

@xxhdx1985126
Copy link
Contributor Author

xxhdx1985126 commented Jan 9, 2018 •
edited

@gregsfortytwo
Or maybe I should move the marking of the initial events of OpRequest out of its constructor?

@gregsfortytwo
Copy link
Member

Yeah, see ad13e05 which introduced that function.

However, that specific part of the code hasn't changed in a long time (2016!) I think something else must be going on.

@liewegas liewegas added the core label Jan 9, 2018
@xxhdx1985126 xxhdx1985126 force-pushed the wip_xxh_trackedop_tracking_start branch from bdea1ab to 040f9c2 Compare January 10, 2018 07:20
@xxhdx1985126 xxhdx1985126 changed the title common: move tracking_start() to TrackedOp's constrcutor common: mark events of TrackedOp outside its constructor Jan 10, 2018
@xxhdx1985126 xxhdx1985126 force-pushed the wip_xxh_trackedop_tracking_start branch 2 times, most recently from 02d55e2 to 9911d78 Compare January 10, 2018 10:00
@liewegas
Copy link
Member

can we put it in create_request<>() instead?

@gregsfortytwo
Copy link
Member

Yes, I like that — all Messages are going to have those fields; we should unify setting the values so it doesn't get lost in future invocations.

@xxhdx1985126 xxhdx1985126 force-pushed the wip_xxh_trackedop_tracking_start branch from 9911d78 to 1bf66de Compare January 11, 2018 02:18
@xxhdx1985126
Copy link
Contributor Author

@gregsfortytwo @liewegas
Hi, it seems that MDRequestImpl marks its initial events with the timestamp in MDRequestImpl::Params while others mark their events with the Message's timestamp, which makes it impossible to uniformly mark their initial events in create_request() without changing the code design structure

@xxhdx1985126 xxhdx1985126 force-pushed the wip_xxh_trackedop_tracking_start branch from 1bf66de to 0681e17 Compare January 11, 2018 11:42
gregsfortytwo
gregsfortytwo reviewed Jan 11, 2018
View reviewed changes
src/msg/Message.h Outdated
virtual const utime_t& get_recv_complete_stamp() const =0;
virtual const utime_t& get_dispatch_stamp() const =0;
virtual ~BasicParams(){};
};
Copy link
Member

Choose a reason for hiding this comment

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

With the templated function I don't think we actually need a shared Params class like this — why did you create it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gregsfortytwo
I intended to use some template argument constraints, c++ concepts for example, to require all params classes to be derived from BasicParams so that they will all have to implement the get_XXX_stamp() methods, which I think is necessary since we will call those methods in create_request.
However, I've just found that c++ concepts is still a work in progress as part of the Concepts Technical Specification that were included in the working draft of C++20, so I guess I should remove BasicParams class. Is this right? If it is, I will remove it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think so. We'll still fail to build if you try and use something that doesn't have those functions. :)

@gregsfortytwo
Copy link
Member

jenkins retest this please

@xuxuehan
common: mark events of TrackedOp outside its constructor
a74e554 
Avoid marking events when TrackedOp::tracking_start() isn't invoked yet.

Fixes: http://tracker.ceph.com/issues/22608
Signed-off-by: Xuehan Xu <xuxuehan@360.cn>
@xxhdx1985126 xxhdx1985126 force-pushed the wip_xxh_trackedop_tracking_start branch from 0681e17 to a74e554 Compare January 13, 2018 17:12
@xxhdx1985126
Copy link
Contributor Author

xxhdx1985126 commented Jan 14, 2018 •
edited

@gregsfortytwo @liewegas I've removed the BasicParams class, please review this pr when you are spare:-)
Thanks:-)

gregsfortytwo
gregsfortytwo requested changes Jan 18, 2018
View reviewed changes
Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

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

The real changes look good here but I think you left in a reference-to-pointer conversion by mistake?

src/mds/Mutation.h
MDRequestImpl(const Params& params, OpTracker *tracker) :
MutationImpl(tracker, params.initiated,
params.reqid, params.attempt, params.slave_to),
MDRequestImpl(const Params* params, OpTracker *tracker) :
Copy link
Member

Choose a reason for hiding this comment

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

Why are you changing this to a const pointer? It was previously a const reference because that's our style guide and makes very clear there won't be any changes or shallow copies of the parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gregsfortytwo
Hi, thanks for the review:-)
I changed this to a pointer because, in create_request, I can only call the get_XXX_stamp() method through either "->" or "." and, in OSD and monitor, the parameter of the create_request method is a pointer, so I choose to change the "params" of MDRequestImpl to a pointer so that I can call the get_XXX_stamp() method uniformly in create_request.

Copy link
Member

Choose a reason for hiding this comment

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

Oh of course, that makes sense,

@gregsfortytwo
Copy link
Member

Would like to run through QA and merge once those extra changes to MutationImpl are gone! :)

@yuriw
Copy link
Contributor

yuriw commented Jan 25, 2018

wip-yuri-testing-2018-01-25-2139

@xxhdx1985126
Copy link
Contributor Author

@yuriw I can't open your card.

@yuriw yuriw merged commit eb55a14 into ceph:master Jan 26, 2018
@xxhdx1985126 xxhdx1985126 deleted the wip_xxh_trackedop_tracking_start branch January 26, 2018 23:31
@smithfarm
Copy link
Contributor

@xxhdx1985126 Maybe you are not a member of https://trello.com/cephstorage yet?

@xxhdx1985126
Copy link
Contributor Author

@smithfarm Yes.
By the way, um..., how can I become a member or a developer of the ceph project? I can't find any guideline about this..... Thanks:-)

@smithfarm
Copy link
Contributor

@xxhdx1985126 Did you try asking on ceph-devel mailing list?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregsfortytwo gregsfortytwo gregsfortytwo approved these changes

Assignees
No one assigned
Labels
core needs-qa wip-yuri-testing
Projects
None yet
Milestone
No milestone
6 participants
@xxhdx1985126 @gregsfortytwo @liewegas @yuriw @smithfarm @xuxuehan