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: add the support for X-History-Location in swift API #15020

Closed
wants to merge 3 commits into from

Conversation

Jing-Scott
Copy link
Contributor

@Jing-Scott
Copy link
Contributor Author

Hi, @rzarzynski , can you help to have a look about this feature?

@rzarzynski rzarzynski self-assigned this May 10, 2017
if (s->info.env->exists("HTTP_X_VERSIONS_LOCATION")) {
bool ver_exists = s->info.env->exists("HTTP_X_VERSIONS_LOCATION");
bool his_exists = s->info.env->exists("HTTP_X_HISTORY_LOCATION");
if (ver_exists && his_exists) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if (struct_v >= 16) {
::decode(swift_versioning, bl);
if (swift_versioning) {
::decode(swift_ver_location, bl);
::decode(swift_his_location, bl);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be performed only for the version 18 and above.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jing-Scott swift_ver_location shouldn't require version 18, right?

Signed-off-by: Jing Wenjun <jingwenjun@cmss.chinamobile.com>
Signed-off-by: Jing Wenjun <jingwenjun@cmss.chinamobile.com>
@Jing-Scott
Copy link
Contributor Author

jekins test this please

@Jing-Scott
Copy link
Contributor Author

@rzarzynski i have rebased and changed to version 18.

@@ -1210,6 +1211,7 @@ struct RGWBucketInfo
::encode(swift_versioning, bl);
if (swift_versioning) {
::encode(swift_ver_location, bl);
::encode(swift_his_location, bl);
Copy link
Contributor

Choose a reason for hiding this comment

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

To preserve compatibility with older struct versions, we should encode new fields at the end, not in the middle. This is required during e.g. cluster upgrades when many instances of RadosGW, possibly from two different releases, may coexist in a single cluster.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rzarzynski seems to be fixed now?

if (swift_versioning) {
::decode(swift_ver_location, bl);
::decode(swift_his_location, bl);
Copy link
Contributor

Choose a reason for hiding this comment

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

This field comes with version 20th, not 18th.

@stale
Copy link

stale bot commented Oct 18, 2018

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If you are a maintainer or core committer, please follow-up on this issue to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Oct 18, 2018
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.

@Jing-Scott I would like to encorporate these improvements, but I think there are problems remaining, e.g., the struct versioning decoding issues @rzarzynski pointed out; if you are willing to update, that would be great!

const string& src_name = src_obj.get_oid();
char buf[src_name.size() + 32];
struct timespec ts = ceph::real_clock::to_timespec(ceph::real_clock::now());
snprintf(buf, sizeof(buf), "%03x%s/%lld.%06ld", (int)src_name.size(),
Copy link
Contributor

Choose a reason for hiding this comment

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

could we use boost::format() boost::str() idioms here?

@@ -1210,6 +1211,7 @@ struct RGWBucketInfo
::encode(swift_versioning, bl);
if (swift_versioning) {
::encode(swift_ver_location, bl);
::encode(swift_his_location, bl);
Copy link
Contributor

Choose a reason for hiding this comment

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

@rzarzynski seems to be fixed now?

if (struct_v >= 16) {
::decode(swift_versioning, bl);
if (swift_versioning) {
::decode(swift_ver_location, bl);
::decode(swift_his_location, bl);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Jing-Scott swift_ver_location shouldn't require version 18, right?

@stale stale bot removed stale labels Nov 1, 2018
@@ -1266,18 +1268,18 @@ struct RGWBucketInfo
}
swift_versioning = false;
swift_ver_location.clear();
if (struct_v >= 16) {
swift_his_location.clear();
if (struct_v >= 18) {
Copy link
Contributor

@rzarzynski rzarzynski Nov 3, 2018

Choose a reason for hiding this comment

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

Something weird happened here. IIUC swift_versioning and swift_ver_location can be present since struct_v 16th but now we expect it only since 18th. I believe we shouldn't touch this but instead introduce:

     swift_his_location.clear();
     if (struct_v >= 20 && swift_versioning) {
       ::decode(swift_his_location, bl);
     }

just before DECODE_FINISH(bl).

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, @rzarzynski !

@stale
Copy link

stale bot commented Feb 11, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
If you are a maintainer or core committer, please follow-up on this issue to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Feb 11, 2019
@stale
Copy link

stale bot commented May 12, 2019

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

@stale stale bot closed this May 12, 2019
@mattbenjamin
Copy link
Contributor

reopen for re-review

@mattbenjamin mattbenjamin reopened this May 12, 2019
@stale stale bot removed the stale label May 12, 2019
@stale
Copy link

stale bot commented Jul 11, 2019

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@stale stale bot added the stale label Jul 11, 2019
@stale
Copy link

stale bot commented Oct 9, 2019

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

@stale stale bot closed this Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants