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.

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

osd: clean up process_peering_events #12009

Merged
merged 1 commit into from Nov 23, 2016
Merged

osd: clean up process_peering_events #12009

merged 1 commit into from Nov 23, 2016

Conversation

363921219
Copy link
Contributor

Signed-off-by: Jie Wang jie.wang@kylin-cloud.com

@ghost
Copy link

ghost commented Nov 16, 2016

jenkins test this please (eio/scrub failure)

@liewegas
Copy link
Member

  1. Are you sure the assert is valid?
  2. Can we drop the curmap assignement inside the loop instead? Or, move curmap scope into the loop?

@ghost
Copy link

ghost commented Nov 16, 2016

jenkins test this please (flake8 related error, now fixed)

@363921219
Copy link
Contributor Author

363921219 commented Nov 17, 2016

@liewegas

Are you sure the assert is valid?

sure, "process_peering_events" will not be called if pgs is empty.

Can we drop the curmap assignement inside the loop instead?

osdmap may be updated when 'process_peering_events' run, so i am not sure it is valid.

Or, move curmap scope into the loop?

curmap is need outside the loop

@tchaikov
Copy link
Contributor

i concur with @363921219, ThreadPool won't pass an empty list to BatchWorkQueue if _void_dequeue() returns NULL.

and dispatch_context(rctx, 0, curmap, &handle) out side of the loop uses the last curmap passed to the last call advance_pg(). so we can compare the curmap.epoch with the nextmap.epoch before sending the messages out.

@@ -9030,7 +9030,8 @@ void OSD::process_peering_events(
{
bool need_up_thru = false;
epoch_t same_interval_since = 0;
OSDMapRef curmap = service.get_osdmap();
assert(!pgs.empty());
Copy link
Contributor

Choose a reason for hiding this comment

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

we can move this assert up into PeeringWQ::_process()`.

Copy link
Contributor

Choose a reason for hiding this comment

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

and also put more details into the commit message: clean up is too vague a word, IMHO.

@363921219
Copy link
Contributor Author

@tchaikov thanks for your comments

@tchaikov
Copy link
Contributor

could you remove "Reviewed-by: Kefu Chai kchai@redhat.com" from your commit messages? it will be added to the merge commit by the one who merges your changes.

@tchaikov
Copy link
Contributor

and please squash these two commit into a single one.

… loops.

    Signed-off-by: Jie Wang <jie.wang@kylin-cloud.com>
@ghost
Copy link

ghost commented Nov 17, 2016

test this please (asok error now fixed in master)

Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

lgtm.

@liewegas liewegas changed the title OSD: clean up process_peering_events osd: clean up process_peering_events Nov 18, 2016
@ghost
Copy link

ghost commented Nov 18, 2016

jenkins test this please (eio)

@tchaikov tchaikov self-assigned this Nov 22, 2016
@tchaikov tchaikov merged commit 354e5bf into ceph:master Nov 23, 2016
@tchaikov
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants