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: copy object add response error messages #18291

Merged
merged 1 commit into from Oct 23, 2017

Conversation

Projects
None yet
4 participants
@ZVampirEM77
Copy link
Contributor

commented Oct 13, 2017

Adding response error messages which are compatible with AWS S3 for copying object operation

Signed-off-by: Enming Zhang enming.zhang@umcloud.com

@amitkumar50
Copy link
Contributor

left a comment

LGTM

@ZVampirEM77

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2017

@joscollin Do you mind to take a review? Thank you~

@joscollin joscollin added the rgw label Oct 16, 2017

@joscollin joscollin self-requested a review Oct 16, 2017

@@ -2103,6 +2103,7 @@ int RGWCopyObj_ObjStore_S3::get_params()
attrs_mod = RGWRados::ATTRSMOD_NONE; // default for intra-zone_group copy
} else {
ldout(s->cct, 0) << "invalid metadata directive" << dendl;
s->err.message = "Unknown metadata directive.";

This comment has been minimized.

Copy link
@joscollin

joscollin Oct 16, 2017

Member

Look at the other functions that does the same. The s->err.message is assigned first, which is fed to ldout. I think you could make this change similar.

@@ -2116,6 +2117,9 @@ int RGWCopyObj_ObjStore_S3::get_params()
/* can only copy object into itself if replacing attrs */
ldout(s->cct, 0) << "can't copy object into itself if not replacing attrs"
<< dendl;
s->err.message = "This copy request is illegal because it is trying to copy "
"an object to itself without changing the object's metadata, "
"storage class, website redirect location or encryption attributes.";

This comment has been minimized.

Copy link
@joscollin

joscollin Oct 16, 2017

Member

ditto. Otherwise, do you have a justification for a different message ? I see that the original message is the brief of your expanded/detailed version.

This comment has been minimized.

Copy link
@ZVampirEM77

ZVampirEM77 Oct 16, 2017

Author Contributor

@joscollin Thanks for your suggestion. I just keep the response error message same with AWS S3 for compatibility.

This comment has been minimized.

Copy link
@ZVampirEM77

ZVampirEM77 Oct 19, 2017

Author Contributor

@joscollin Do you still think I should keep s->err.message same with ldout? If so, I can change my commit in your way~ Thank you~

This comment has been minimized.

Copy link
@joscollin

joscollin Oct 19, 2017

Member

I would say, you can keep the s->err.message text the same to be compatible with AWS S3. But I was suggesting that you could change it this way, instead of two different texts. What do you think ?

This comment has been minimized.

Copy link
@ZVampirEM77

ZVampirEM77 Oct 20, 2017

Author Contributor

OK

rgw: copy object add response error message
Signed-off-by: Enming Zhang <enming.zhang@umcloud.com>

@ZVampirEM77 ZVampirEM77 force-pushed the ZVampirEM77:wip-copy-errormsg branch from f701ad8 to 7b8502c Oct 20, 2017

@ZVampirEM77

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2017

@joscollin your suggestion is very appreciated. I have recommitted it. Do you mind to review it again?

@ZVampirEM77

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2017

@joscollin ping

@yuriw

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2017

@yuriw yuriw merged commit c3d84fa into ceph:master Oct 23, 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.