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 support for RGWLifecycleConfiguration #18959

Merged
merged 1 commit into from Nov 29, 2017

Conversation

Projects
None yet
3 participants
@cooboos
Copy link

commented Nov 16, 2017

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

@cooboos cooboos force-pushed the cooboos:wip-cephdencoder-LCtype branch from adbb810 to 3ec00e3 Nov 16, 2017

map<string, lc_op>::const_iterator iter = prefix_map.begin();
f->open_array_section("prefix_map");
for (; iter != prefix_map.end(); ++iter) {
f->dump_string("prefix", iter->first);

This comment has been minimized.

Copy link
@liewegas

liewegas Nov 16, 2017

Member

the items in the array shoudl all have the same type. if you're trying to make a map, do open_object_section outside the loop, and inside the loop to open_object_section(iter->first.c_str()).

This comment has been minimized.

Copy link
@cooboos

cooboos Nov 16, 2017

Author

Thanks @liewegas . I have correct it.
But I have got the same mismatch type in both RGWObjManifest::dump and RGWAccessControlList::dump;Should I pull another new request to correct that ?

This comment has been minimized.

Copy link
@cooboos

cooboos Nov 17, 2017

Author

sorry @liewegas , It's my mistake. I misunderstood you suggestion, I have correct it.

@liewegas liewegas added the rgw label Nov 16, 2017

@cooboos cooboos force-pushed the cooboos:wip-cephdencoder-LCtype branch 2 times, most recently from 0958a75 to f2be811 Nov 16, 2017

multi_iter->second.dump(f);
f->close_section();
f->close_section();
}

This comment has been minimized.

Copy link
@liewegas

liewegas Nov 17, 2017

Member

it's a matter of taste, but you can also do

+  f->open_object_section("rule_map");
 +  for (; multi_iter != rule_map.end(); ++multi_iter) {
 +    f->open_object_section(multi_iter->first.c_str());
 +    multi_iter->second.dump(f);
 +    f->close_section();
 +  }
 +  f->close_section();

This comment has been minimized.

Copy link
@liewegas

liewegas Nov 17, 2017

Member

or even

+  f->open_object_section("rule_map");
 +  for (; multi_iter != rule_map.end(); ++multi_iter) {
 +    f->dump_object(multi_iter->first.c_str(), multi_iter->second);
 +  }
 +  f->close_section();

This comment has been minimized.

Copy link
@cooboos

cooboos Nov 17, 2017

Author

@liewegas Thank you for your suggestion, I have fixed it. Please check, when you get a chance.

@cooboos cooboos force-pushed the cooboos:wip-cephdencoder-LCtype branch from f2be811 to 7b52947 Nov 17, 2017

f->close_section();

multimap<string, LCRule>::const_iterator multi_iter = rule_map.begin();
f->open_object_section("rule_map");

This comment has been minimized.

Copy link
@liewegas

liewegas Nov 17, 2017

Member

here using an object isn't a good idea since it's a multimap<>, so there might be duplicate rule names?

This comment has been minimized.

Copy link
@cooboos

cooboos Nov 18, 2017

Author

@liewegas Thank you, Please check again. The last change is the map usging an object , and the multimap using an array.

@cooboos cooboos force-pushed the cooboos:wip-cephdencoder-LCtype branch from 7b52947 to 4ad7966 Nov 18, 2017

encode_json("expiration", expiration, f);
encode_json("noncur_expiration", noncur_expiration, f);
encode_json("mp_expiration", mp_expiration, f);
if (expiration_date != boost::none) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 22, 2017

Contributor

nit, could just put if (obj_tags).

utime_t ut(*expiration_date);
encode_json("expiration_date", ut, f);
}
if (obj_tags != boost::none) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 22, 2017

Contributor

ditto.

{
map<string, lc_op>::const_iterator iter = prefix_map.begin();
f->open_object_section("prefix_map");
for (; iter != prefix_map.end(); ++iter) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 22, 2017

Contributor

use range-based loop, if you please.


multimap<string, LCRule>::const_iterator multi_iter = rule_map.begin();
f->open_array_section("rule_map");
for (; multi_iter != rule_map.end(); ++multi_iter) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 22, 2017

Contributor

ditto.

@@ -61,7 +61,7 @@ class LCExpiration
DECODE_FINISH(bl);
}
void dump(Formatter *f) const;
// static void generate_test_instances(list<ACLOwner*>& o);
//static void generate_test_instances(list<ACLOwner*>& o);

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 22, 2017

Contributor

please drop this change if it's not relevant.


void lc_op::dump(Formatter *f) const
{
encode_json("status", status, f);

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 22, 2017

Contributor

why are you hardwiring this to json? can we use formatter's method?

@cooboos cooboos force-pushed the cooboos:wip-cephdencoder-LCtype branch from 4ad7966 to 104ec50 Nov 23, 2017

@cooboos

This comment has been minimized.

Copy link
Author

commented Nov 23, 2017

@tchaikov Thanks for your suggestion, pls check again.

f->dump_int("mp_expiration", mp_expiration);
if (expiration_date) {
utime_t ut(*expiration_date);
encode_json("expiration_date", ut, f);

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 23, 2017

Contributor

use dump_object() please.

This comment has been minimized.

Copy link
@cooboos

cooboos Nov 24, 2017

Author

@tchaikov Sorry, but there is no definition about utime_t::dump(). Do I need to define a new utime_t::dump() ? or transform utime_t to string ? or use f->dump_stream ?
appreciate for your comments.

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 24, 2017

Contributor

@cooboos ahh, right! i didn't realize that utime_t does not have this method ATM. probably a better alternative is f->dump_stream(). thanks!

{
for (const auto& tag: tag_map){
f->open_object_section("tag_map");
f->dump_string("Key", tag.first);

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 23, 2017

Contributor

i am curious about why you are using the capitalized "key"?


void RGWObjTags::dump(Formatter *f) const
{
for (const auto& tag: tag_map){

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 23, 2017

Contributor

nit, it's const already, b/c this method is const.

@cooboos cooboos force-pushed the cooboos:wip-cephdencoder-LCtype branch 2 times, most recently from d3a3fa9 to 649391a Nov 24, 2017

@cooboos

This comment has been minimized.

Copy link
Author

commented Nov 24, 2017

@tchaikov pls check again.

@tchaikov

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2017

also your "Signed-off-by" line does not look right.

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

see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#1-sign-your-work

@cooboos cooboos force-pushed the cooboos:wip-cephdencoder-LCtype branch from 649391a to 315692c Nov 24, 2017

@cooboos

This comment has been minimized.

Copy link
Author

commented Nov 24, 2017

@tchaikov OK, you are so appreciated.

@tchaikov

This comment has been minimized.

Copy link
Contributor

commented Nov 24, 2017

still does not look right, to be specific

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

please note that the email is quoted using bracket. if your git is configured correctly, git commit -s will sign off your commit automatically.

f->dump_stream("expiration_date") << ut;
}
if (obj_tags) {
encode_json("obj_tags", *obj_tags, f);

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 24, 2017

Contributor

might want to use dump_object() instead.

void LCFilter::dump(Formatter *f) const
{
f->dump_string("prefix", prefix);
encode_json("obj_tags", obj_tags, f);

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 24, 2017

Contributor

ditto.

f->dump_string("id", id);
f->dump_string("prefix", prefix);
f->dump_string("status", status);
encode_json("expiration", expiration, f);

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 24, 2017

Contributor

ditto.

@cooboos cooboos force-pushed the cooboos:wip-cephdencoder-LCtype branch from 315692c to 15fcf0f Nov 24, 2017

@cooboos

This comment has been minimized.

Copy link
Author

commented Nov 24, 2017

@tchaikov you are a good reviewer, thank you. Check again pls.

@@ -13,6 +13,7 @@
#include "rgw_common.h"
#include "rgw_bucket.h"
#include "rgw_lc.h"
#include "common/ceph_json.h"

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 24, 2017

Contributor

why include this header here? the dump() method is defined in src/rgw/rgw_json_enc.cc.

This comment has been minimized.

Copy link
@cooboos

cooboos Nov 24, 2017

Author

@tchaikov I defined dump() method in other file previously, then forgot to remove it. so...
Thanks anyway, check pls.

@cooboos cooboos force-pushed the cooboos:wip-cephdencoder-LCtype branch from 15fcf0f to 886feef Nov 24, 2017

@tchaikov
Copy link
Contributor

left a comment

modulo the comments, lgtm.


void RGWLifecycleConfiguration::dump(Formatter *f) const
{
map<string, lc_op>::const_iterator iter = prefix_map.begin();

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 26, 2017

Contributor

please remove this line.

{
map<string, lc_op>::const_iterator iter = prefix_map.begin();
f->open_object_section("prefix_map");
for (auto& iter : prefix_map) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 26, 2017

Contributor

iter is not an iterator. might want to s/iter/op/.

}
f->close_section();

multimap<string, LCRule>::const_iterator multi_iter = rule_map.begin();

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 26, 2017

Contributor

again, please drop this line.


multimap<string, LCRule>::const_iterator multi_iter = rule_map.begin();
f->open_array_section("rule_map");
for (auto& multi_iter : rule_map) {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Nov 26, 2017

Contributor

s/multi_iter/rule/

wangsongbo
ceph-dencoder: add support for RGWLifecycleConfiguration
Signed-off-by: Songbo Wang wangsongbo@cloudin.cn

@cooboos cooboos force-pushed the cooboos:wip-cephdencoder-LCtype branch from 886feef to 35cb7ac Nov 27, 2017

@cooboos

This comment has been minimized.

Copy link
Author

commented Nov 27, 2017

@tchaikov check pls.

@tchaikov tchaikov merged commit f39bff5 into ceph:master Nov 29, 2017

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.