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

rgw: obsolete 'radosgw-admin period prepare' command #11278

Merged

Conversation

gaurav36
Copy link
Contributor

All that the 'radosgw-admin period prepare' command does is create an
empty period and write it to rados. When we added the staging period
(see 'period get --staging'), uncommitted changes were written to that
instead - so the 'period prepare' command became obsolete. It should be
removed!

Fixes: http://tracker.ceph.com/issues/17387

Reported-by: Casey Bodley cbodley@redhat.com
Signed-off-by: Gaurav Kumar Garg garg.gaurav52@gmail.com

@gaurav36 gaurav36 force-pushed the wip-obsolete-rgw-admin-period-prepare branch from d9481dc to a536a02 Compare September 30, 2016 12:14
if (strcmp(cmd, "prepare") == 0) {
cerr <<"command is obsolete; please check usage and/or man page" <<std::endl;
return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think it's necessary to keep this message. could you also remove OPT_PERIOD_PREPARE from the enum?

Copy link
Contributor Author

@gaurav36 gaurav36 Sep 30, 2016

Choose a reason for hiding this comment

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

@cbodley Thank you
I will remove OPT_PERIOD_PREPARE in next patch.
If we remove the message then in next release if user execute period prepare command then he might wonder about period prepare command, means what happened to this command. Should we put any other message or completely remove this message (" command is obsolete; please check usage and/or man page") ?

Copy link
Contributor

Choose a reason for hiding this comment

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

i appreciate you wanting to document the deprecation, but the reason i don't think it's necessary is that we've never documented this command (outside of radosgw-admin --help) as part of the multisite configuration process

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbodley thanks,
right, i will do it in next patch set.

@cbodley cbodley added the rgw label Sep 30, 2016
@cbodley cbodley self-assigned this Sep 30, 2016
@gaurav36 gaurav36 force-pushed the wip-obsolete-rgw-admin-period-prepare branch from a536a02 to 7b39482 Compare October 4, 2016 08:32
@gaurav36
Copy link
Contributor Author

gaurav36 commented Oct 4, 2016

@cbodley test case failure for this PR seems to be spurious failure. I saw same test failure on some other PR as well which have nothing to do with code change.

return OPT_PERIOD_PREPARE;
if (strcmp(cmd, "prepare") == 0) {
return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove this block entirely now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbodley ahhh, i missed it.

All that the 'radosgw-admin period prepare' command does is create an
empty period and write it to rados. When we added the staging period
(see 'period get --staging'), uncommitted changes were written to that
instead - so the 'period prepare' command became obsolete. It should be
removed!

Fixes: http://tracker.ceph.com/issues/17387

Reported-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Gaurav Kumar Garg <garg.gaurav52@gmail.com>
@gaurav36 gaurav36 force-pushed the wip-obsolete-rgw-admin-period-prepare branch from 7b39482 to 1508c70 Compare October 4, 2016 14:01
@cbodley cbodley merged commit 8c2dc6e into ceph:master Oct 4, 2016
@cbodley
Copy link
Contributor

cbodley commented Oct 4, 2016

thank you!

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