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: need to 'open_object_section' before dump stats in 'RGWGetUsage_… #9495

Closed
wants to merge 1 commit into from

Conversation

weiqiaomiao
Copy link
Contributor

…ObjStore_S3::send_response()' function

Signed-off-by: weiqiaomiao wei.qiaomiao@zte.com.cn

@yehudasa
Copy link
Member

yehudasa commented Jun 4, 2016

@weiqiaomiao this is a public api (albeit rgw extension). Need to bring this change to the mailing list, people might be using it as is now.

@yehudasa
Copy link
Member

yehudasa commented Jun 4, 2016

@chenji-kael please take a look at this one

@chenji-kael
Copy link
Contributor

@yehudasa @weiqiaomiao
Thanks for your remind, I think it's just about json decode format issue, and does not much matter, and actually we use this api for production environment ,and change this may cause problems when we update ceph.

@weiqiaomiao
Copy link
Contributor Author

@yehudasa I send a email to the mailing list(the email titile is 'RGW:add description for GET usage admin api response entity 'stats entry''), and there is no one reponse it. I think there is no people using this api except chenji.
@chenji-kael One of our customers planning use this api, and it is comfused to them what's the figure mean when they decode the api reponse data, add a description is a simple but easy way to help them quickly understand it.How about to fix this when you convenience?

@chenji-kael
Copy link
Contributor

@weiqiaomiao
please see this pr , maybe you need it
#8043

@yehudasa
Copy link
Member

@weiqiaomiao @chenji-kael I suggest that we switch to the new api, but make it configurable so that current users can fall back to the old api. What do you think?

@weiqiaomiao
Copy link
Contributor Author

@yehudasa @chenji-kael It is a good suggestion to me and update the commit yet.

@@ -1433,6 +1433,8 @@ OPTION(rgw_swift_versioning_enabled, OPT_BOOL, false) // whether swift object ve

OPTION(rgw_list_bucket_min_readahead, OPT_INT, 1000) // minimum number of entries to read from rados for bucket listing

OPTION(rgw_getusage_dump_description, OPT_BOOL, true) // dump description of total stats for s3 GetUsage API
Copy link
Member

Choose a reason for hiding this comment

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

@weiqiaomiao let's rename it to rgw_rest_getusage_op_compat, and default it to false (switch the flag's meaning)

@weiqiaomiao
Copy link
Contributor Author

@yehudasa Thanks for your suggestion, and the commit has been updated. Would you take a look at it again?

@yehudasa
Copy link
Member

@weiqiaomiao this looks ok. Last thing we need is that since this affects compatibility, we need to note it in the relase notes. Please add a description of this change, the new configurable (to override), and the api change to ceph/PendingReleaseNotes. Thanks.

@weiqiaomiao weiqiaomiao force-pushed the wqm-wip-rgw-getusage branch 2 times, most recently from 2da1e22 to e25a6f0 Compare July 25, 2016 11:06
@weiqiaomiao
Copy link
Contributor Author

@yehudasa I already add this change to ceph/PendingReleaseNote. Would you take a look at it again.Thanks.

@@ -17,6 +17,24 @@
listed in 'osd class default list' requires a capability naming the class
(e.g. 'allow class foo').

* The 'rgw_rest_getusage_op_compat' config option allow you to dump the description of user stats
Copy link
Member

Choose a reason for hiding this comment

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

config option allows

Copy link
Member

Choose a reason for hiding this comment

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

@weiqiaomiao also, no underscores needed when specifying the config option

@weiqiaomiao
Copy link
Contributor Author

@theanalyst the commit has been updated according to your suggestion. would you take a look at it again. thanks!

@yehudasa
Copy link
Member

@weiqiaomiao please note the two comments on the last commit

…ObjStore_S3::send_response()' function

Signed-off-by: weiqiaomiao <wei.qiaomiao@zte.com.cn>
@weiqiaomiao
Copy link
Contributor Author

@yehudasa Thank you for reminding me. The commit has been updated.

@yehudasa
Copy link
Member

yehudasa commented Oct 5, 2016

Created and merged another PR due to conflict, see #11325

@yehudasa yehudasa closed this Oct 5, 2016
@yehudasa
Copy link
Member

yehudasa commented Oct 5, 2016

@weiqiaomiao also updated commit message, noted tracker issue

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