rgw:lc fix expiration time #17824
rgw:lc fix expiration time #17824
Conversation
src/rgw/rgw_lc_s3.cc
Outdated
| @@ -32,7 +32,9 @@ bool LCExpiration_S3::xml_end(const char * el) { | |||
| } else { | |||
| date = lc_date->get_data(); | |||
| //We need return xml error according to S3 | |||
| if (boost::none == ceph::from_iso_8601(date)) { | |||
| boost::optional<ceph::real_time> expiration_date = ceph::from_iso_8601(date); | |||
| timespec expiration_time = ceph::real_clock::to_timespec(*expiration_date); | |||
There was a problem hiding this comment.
better off dereferencing boost::optional<> after checking it by comparing it with boost::none.
There was a problem hiding this comment.
Did you mean add expiration_date = boost::none; after comparing?
Why better off dereferncing here?
There was a problem hiding this comment.
Yeah, I also want to know why? It is just a local variable.
There was a problem hiding this comment.
if from_iso_8601() returns boost::none, ceph::real_clock::to_timespec(*expiration_date) will trigger an assertion failure.
There was a problem hiding this comment.
yes, thanks. I'll revise it later.
|
@dang Could you review it for me, thx. |
…nfiguration xml According to S3, expiration_date must be midnight. Signed-off-by: Shasha Lu <lu.shasha@eisoo.com>
shashalu
force-pushed
the
fix-lc-expiration
branch
from
9b44886
to
7038872
Compare
|
@shashalu , can you open a tracker issue? |
shashalu
force-pushed
the
fix-lc-expiration
branch
from
7038872
to
94913da
Compare
|
@oritwas Already updated. |
src/rgw/rgw_lc.cc
Outdated
| @@ -299,8 +299,11 @@ int RGWLC::handle_multipart_expiration(RGWRados::Bucket *target, const map<strin | |||
| } | |||
|
|
|||
| utime_t now = ceph_clock_now(); | |||
| utime_t base_time = now; | |||
| if (cct->_conf->rgw_lc_debug_interval <= 0) | |||
There was a problem hiding this comment.
All this code can be done once in obj_has_expired(), rather than being done separately all over the place. That's why obj_has_expired() was added in the first place.
There was a problem hiding this comment.
@dang Already updated.
| @@ -32,7 +32,12 @@ bool LCExpiration_S3::xml_end(const char * el) { | |||
| } else { | |||
| date = lc_date->get_data(); | |||
| //We need return xml error according to S3 | |||
| if (boost::none == ceph::from_iso_8601(date)) { | |||
| boost::optional<ceph::real_time> expiration_date = ceph::from_iso_8601(date); | |||
There was a problem hiding this comment.
I'm ambivalent about this. Just because Amazon limits the time to midnight doesn't mean we should, necessarily. I'd prefer to truncate the time to midnight, if necessary, rather than throw an error. I can be convinced otherwise, though.
There was a problem hiding this comment.
In my opinion, I'd prefer to keep the same with s3.
shashalu
force-pushed
the
fix-lc-expiration
branch
3 times, most recently
from
0c57b20
to
552379c
Compare
Amazon S3 calculates the expiration time by adding the number of days specified in the rule to the object creation time and rounding the resulting time to the next day midnight. So rgw should using the rounding down midnight time to compare with the object creation time to check obj expiration. Fixes: http://tracker.ceph.com/issues/21533 Signed-off-by: Shasha Lu <lu.shasha@eisoo.com>
shashalu
force-pushed
the
fix-lc-expiration
branch
from
552379c
to
4a759c9
Compare
1、According to S3, expiration_date must be midnight.
2、Amazon S3 calculates the expiration time by adding the number of days specified in the rule to the object creation time and rounding the resulting time to the next day midnight.
So rgw should using the rounding down midnight time to compare with the object creation time to check obj expiration.
Signed-off-by: Shasha Lu lu.shasha@eisoo.com