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_lc: add support for optional filter argument and make ID optional #16818

Merged
merged 1 commit into from Aug 22, 2017

Conversation

Projects
None yet
6 participants
@theanalyst
Copy link
Member

commented Aug 4, 2017

Support Filter tag in Lifecycle XML similar to AWS S3, while S3 docs
mention that this tag is mandatory, older clients still default to
Prefix, and S3 itself seems to relaxed in enforcing the rule, this
implementation also follows a similar pattern. Filter optionally
supports filtering via (multiple) Object Tags, this is still a TODO. The
current implementation of object tags is still as an object xattr, and
since the LC processing still iterates over the bucket index which
currently doesn't have any info. on tags, this requires some thought
into for implementing without a larger performance penalty

Fixes: http://tracker.ceph.com/issues/20872 & http://tracker.ceph.com/issues/19587
Signed-off-by: Abhishek Lekshmanan abhishek@suse.com

@theanalyst theanalyst requested a review from dang Aug 4, 2017

@theanalyst theanalyst force-pushed the theanalyst:rgw-lc-add-filter-2 branch 2 times, most recently from fc608c5 to e353fb7 Aug 4, 2017

@dang

dang approved these changes Aug 4, 2017

} else {
prefix = rule->get_prefix();
}
auto ret = prefix_map.insert(pair<string, lc_op>(prefix, op));

This comment has been minimized.

Copy link
@theanalyst

theanalyst Aug 7, 2017

Author Member

insert -> emplace

@theanalyst theanalyst force-pushed the theanalyst:rgw-lc-add-filter-2 branch from e353fb7 to 8d8c753 Aug 11, 2017

@theanalyst theanalyst changed the title wip: rgw_lc: add support for optional filter argument and make ID optional rgw_lc: add support for optional filter argument and make ID optional Aug 11, 2017

@theanalyst

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2017

changelog: moved from insert to emplace for prefix_map

Also added a test cases in s3tests ceph/s3-tests#180

@theanalyst

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2017

@dang can you take a look again

std::string prefix;
// TODO add support for tagging
public:
std::string& get_prefix(){

This comment has been minimized.

Copy link
@chardan

chardan Aug 14, 2017

Contributor

You can also add const and nothrow to accessors like this.

This comment has been minimized.

Copy link
@chardan

chardan Aug 14, 2017

Contributor

(I guess come to think of it, is there a real point to having an accessor or mutator here at all? If this isn't being used through some interface somewhere, maybe it's more straightforward without them. Seems ok as-is, but something to consider.)

} else {
prefix = rule->get_prefix();
}
auto ret = prefix_map.emplace(pair<string, lc_op>(prefix, op));

This comment has been minimized.

Copy link
@chardan

chardan Aug 14, 2017

Contributor

Nothing wrong with this, but:
return prefix_map[prefix] = op;
...feels simpler to me.

This comment has been minimized.

Copy link
@theanalyst

theanalyst Aug 17, 2017

Author Member

moved to using std::move & dropping creating a pair since the vars are local and have no use anymore

@@ -164,21 +206,26 @@ class LCRule
dm_expiration = _dm_expiration;
}

void set_filter(LCFilter* _filter){

This comment has been minimized.

Copy link
@chardan

chardan Aug 14, 2017

Contributor

Is it ok if _filter is nullptr?

This comment has been minimized.

Copy link
@theanalyst

theanalyst Aug 16, 2017

Author Member

good catch, ideally I guess this would be better as a boost::optional maybe, but will check for nullptr now

This comment has been minimized.

Copy link
@theanalyst

theanalyst Aug 17, 2017

Author Member

this setter is actually unused, since the only caller is from LCRule, I'll drop this

@@ -180,6 +220,8 @@ XMLObj *RGWLCXMLParser_S3::alloc_obj(const char *el)
obj = new LCRule_S3();
} else if (strcmp(el, "ID") == 0) {
obj = new LCID_S3();
} else if (strcmp(el, "Filter") == 0) {

This comment has been minimized.

Copy link
@chardan

chardan Aug 14, 2017

Contributor

With this many branches and strcmp() calls, I start to wonder if it's time to rethink the approach (maybe it's not quite the time today, but...).

@theanalyst

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2017

@dang ping

@theanalyst theanalyst force-pushed the theanalyst:rgw-lc-add-filter-2 branch from 8d8c753 to e8460b4 Aug 16, 2017

@theanalyst

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2017

changelog: filter setter checks for null

@theanalyst theanalyst requested a review from yehudasa Aug 16, 2017

ENCODE_FINISH(bl);
}
void decode(bufferlist::iterator& bl) {
DECODE_START_LEGACY_COMPAT_LEN(1, 1, 1, bl);

This comment has been minimized.

Copy link
@yehudasa

yehudasa Aug 16, 2017

Member

this is not a legacy encoded object, should just use DECODE_START()

This comment has been minimized.

Copy link
@theanalyst

theanalyst Aug 17, 2017

Author Member

ack

@@ -67,6 +67,17 @@ bool RGWLifecycleConfiguration_S3::xml_end(const char *el) {
return true;
}

bool LCFilter_S3::xml_end(const char* el) {

This comment has been minimized.

Copy link
@yehudasa

yehudasa Aug 16, 2017

Member

Note that the whole xml parsing can be simplified by using RGWXMLDecoder (see rgw_xml_enc.cc)

@theanalyst theanalyst force-pushed the theanalyst:rgw-lc-add-filter-2 branch from 68e8f09 to 8bdf595 Aug 17, 2017

rgw_lc: add support for optional filter argument and make ID optional
Support Filter tag in Lifecycle XML similar to AWS S3, while S3 docs
mention that this tag is mandatory, older clients still default to
Prefix, and S3 itself seems to relaxed in enforcing the rule, this
implementation also follows a similar pattern. Filter optionally
supports filtering via (multiple) Object Tags, this is still a TODO. The
current implementation of object tags is still as an object xattr, and
since the LC processing still iterates over the bucket index which
currently doesn't have any info. on tags, this requires some thought
into for implementing without a larger performance penalty

Fixes: http://tracker.ceph.com/issues/20872
Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>

@theanalyst theanalyst force-pushed the theanalyst:rgw-lc-add-filter-2 branch from 8bdf595 to 93a8583 Aug 17, 2017

@theanalyst

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2017

@yehudasa updated

@theanalyst

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2017

jenkins test this please

@theanalyst

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2017

jenkins test docs

@mattbenjamin

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2017

@theanalyst ready to merge? Luminous?

@theanalyst

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2017

@mattbenjamin Yehuda had left some review comments regarding moving to xml_decoder , I've addressed those, can you take a look again @yehudasa

@theanalyst

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2017

@mattbenjamin @yehudasa is this ok to go?

@yuriw yuriw merged commit c5ee282 into ceph:master Aug 22, 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.