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

ceph-dencoder: add missing begin_iter & end_iter item for RGWObjManifest #19509

Merged
merged 1 commit into from Apr 26, 2018

Conversation

cooboos
Copy link

@cooboos cooboos commented Dec 14, 2017

Signed-off-by: Songbo Wang wangsongbo@cloudin.cn

Signed-off-by: Songbo Wang wangsongbo@cloudin.cn
Copy link
Contributor

@mattbenjamin mattbenjamin left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -107,6 +127,9 @@ void RGWObjManifest::dump(Formatter *f) const
::encode_json("rules", rules, f);
::encode_json("tail_instance", tail_instance, f);
::encode_json("tail_placement", tail_placement, f);

f->dump_object("begin_iter", begin_iter);
Copy link
Contributor

Choose a reason for hiding this comment

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

should be able to use encode_json() instead, probably better using it instead of all the direct formatter->dump() calls

Copy link
Author

@cooboos cooboos Dec 14, 2017

Choose a reason for hiding this comment

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

@yaozongyou in my previous pr(#18959 ), @tchaikov suggest to use formatter's method. It confused me which should be used.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

@yaozongyou well, it depends on if we are dumping a JSON or not. but in this case, i think RGWObjManifest::dump() is a method accepting a general Formatter, why shall we use encode_json()?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tchaikov encode_json's last parameter is a general ceph::Formatter,it has the same capability with formatter->dump. and in this source code file, as the original GWObjManifest::dump using encode_json, we'd better keeping consistent with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

it has the same capability with formatter->dump.

also please note that the same applies to to encode_json(). and the latter is a wrapper of the formatter->dump(), extra cpu cycles wasted if not inlined.

as the original GWObjManifest::dump using encode_json, we'd better keeping consistent with it.

Foo::dump() is a general interface used by Formatter not just JSON dump methods, it's consistent to follow this convention. but i understand, the to use encode_json() is also consistent with the rest part of this method or this source file. so no strong feelings either way. the reason i am chiming in here is merely that i am @'ed.

Copy link
Contributor

Choose a reason for hiding this comment

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

as @tchaikov has no favor for either way and @mattbenjamin has approved it, @cooboos let's keep it as it is.

Copy link
Author

Choose a reason for hiding this comment

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

@yaozongyou ok, thank you !

@tchaikov
Copy link
Contributor

retest this please

@tchaikov
Copy link
Contributor

retest this please.

@mattbenjamin
Copy link
Contributor

@cooboos are you adding the iterator format for debugging or tracing purposes? I think that's it's only utility, but adding @yehudasa to cross check.

@yuriw
Copy link
Contributor

yuriw commented Feb 14, 2018

test this please

@yuriw
Copy link
Contributor

yuriw commented Apr 25, 2018

wip-yuri3-testing

@yuriw yuriw merged commit 7aa5689 into ceph:master Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants