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

mon: osdmap's epoch should be more than 0 #9859

Merged
merged 1 commit into from Nov 25, 2016
Merged

mon: osdmap's epoch should be more than 0 #9859

merged 1 commit into from Nov 25, 2016

Conversation

x11562
Copy link

@x11562 x11562 commented Jun 22, 2016

Signed-off-by: Na Xie xie.na@h3c.com

@@ -3106,7 +3106,7 @@ bool OSDMonitor::preprocess_command(MonOpRequestRef op)
int64_t epochnum;
cmd_getval(g_ceph_context, cmdmap, "epoch", epochnum, (int64_t)0);
epoch = epochnum;
if (!epoch)
if (epoch > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this change make any difference? or it is just cosmetic?

Copy link
Author

Choose a reason for hiding this comment

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

ceph osd getmap 0

such as command, get the latest osdmap.
this change tells that 0 is invalid.

Copy link
Contributor

Choose a reason for hiding this comment

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

this change breaks the behaviour of
ceph osd getmap.

in this case, epoch is 0.

Copy link
Author

@x11562 x11562 Jun 29, 2016

Choose a reason for hiding this comment

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

0 is invalid,
epoch started from 1

Copy link
Contributor

Choose a reason for hiding this comment

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

the default value of epoch is "0" if it's not specified. we use it to tell if user wants to get the latest osdmap.

with your change, if user puts ceph osd getmap. he/she will get an error.

$ ./bin/ceph osd getmap
Error ENOENT: there is no map for epoch 0

Copy link
Contributor

Choose a reason for hiding this comment

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

further more, you are overwriting the epoch passed by user with the latest one. this does not sound right.

Copy link
Author

Choose a reason for hiding this comment

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

if epoch is not specified, the latest epoch returned,rather than setted to 0,
what do you think?

Signed-off-by: Na Xie <xie.na@h3c.com>
@ghost
Copy link

ghost commented Nov 22, 2016

jenkins test this please (jenkins hung)

Copy link
Member

@jecluis jecluis left a comment

Choose a reason for hiding this comment

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

I believe, as it is, this commit is purely cosmetic. Functionally it is the same as the original code. I am not opposed to it, but I'm lacking the motivation for the change besides making it different.

@tchaikov thoughts?

@tchaikov
Copy link
Contributor

@jecluis agreed. but the change improves the readability, because the last param of cmd_getval() is the default value of the val of the command option, and we are using "0" as a flag actually. so i lean towards merging it after running it through rados qa.

@jecluis
Copy link
Member

jecluis commented Nov 22, 2016

@tchaikov fair enough. thumbsup.png

@tchaikov tchaikov merged commit e2c848d into ceph:master Nov 25, 2016
@tchaikov
Copy link
Contributor

@x11562 x11562 deleted the OSDMap branch January 6, 2017 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants