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: check placement existence when create bucket #16385

Merged
merged 1 commit into from Jul 25, 2017

Conversation

Projects
None yet
4 participants
@mikulely
Contributor

mikulely commented Jul 18, 2017

Signed-off-by: Jiaying Ren jiaying.ren@umcloud.com

Related to #15790

@mikulely mikulely changed the title from rgw: checkout placement existness when create bucket to rgw: checkout placement existence when create bucket Jul 18, 2017

@mikulely mikulely changed the title from rgw: checkout placement existence when create bucket to rgw: check placement existence when create bucket Jul 18, 2017

@joscollin

src/rgw/rgw_rados.h

You have already made the change for rgw_rados.h in PR#16384.

@mikulely

This comment has been minimized.

Contributor

mikulely commented Jul 18, 2017

Thanks @joscollin for reminding this, it's intended to make review easier, will resolve the conflict when one of them been merged.

@joscollin

This comment has been minimized.

Member

joscollin commented Jul 18, 2017

@mikulely I think you can make it a single PR that contains both the changes and specifying both the requirements in the Description. This avoids the duplicate and it would be neat. What do you say ?

@mikulely

This comment has been minimized.

Contributor

mikulely commented Jul 18, 2017

@joscollin I'm not sure . As #16385 may need to backport ,but #16384 maybe not.

std::string rule;
if (!request_rule.empty()) {
if (zonegroup.has_placement_target(request_rule)) {

This comment has been minimized.

@cbodley

cbodley Jul 18, 2017

Contributor

note that these calls to has_placement_target() are duplicated with the final call to zonegroup.placement_targets.find(rule);. what do you think about tracking the selected rule by its iterator, instead of std::string rule? something like this:

  std::map<std::string, RGWZoneGroupPlacementTarget>::const_iterator titer;

  if (!request_rule.empty()) {
    titer = zonegroup.placement_targets.find(request_rule);
    if (titer == zonegroup.placement_targets.end()) {
      ldout(cct, 0) << "could not find requested placement id " << request_rule << " within zonegroup " << dendl;
      return -EINVAL;
    }
  } else if (!user_info.default_placement.empty()) {
    titer = zonegroup.placement_targets.find(user_info.default_placement);
    if (titer == zonegroup.placement_targets.end()) {
      ldout(cct, 0) << "could not find user default placement id " << user_info.default_placement << " within zonegroup " << dendl;
      return -EINVAL;
    }
  } ...

that avoids duplicating the final search through zonegroup.placement_targets, as well as the string copies into std::string rule. the new RGWZoneGroup::has_placement_target() also becomes unnecessary

This comment has been minimized.

@mikulely

mikulely Jul 19, 2017

Contributor

Nice idea, thanks.

if (zonegroup.has_placement_target(request_rule)) {
rule = request_rule;
} else {
ldout(cct, 0) << "could not find requested placement id " << rule << " within zonegroup " << dendl;

This comment has been minimized.

@cbodley

cbodley Jul 18, 2017

Contributor

this should be request_rule, as rule isn't initialized in this branch

rule = user_info.default_placement;
} else {
ldout(cct, 0) << "could not find user default placement id " << rule << " within zonegroup " << dendl;
return -EINVAL;

This comment has been minimized.

@cbodley

cbodley Jul 18, 2017

Contributor

should these be -ERR_INVALID_LOCATION_CONSTRAINT instead?

This comment has been minimized.

@mikulely

mikulely Jul 19, 2017

Contributor

@cbodley Agree, By using ERR_INVALID_LOCATION_CONSTRAINT user will get the S3 error code, which can clearly point out the error reason. I've also add ERR_ZONEGROUP_DEFAULT_PLACEMENT_MISCONFIGURATION to instead EIO. what do you think?

rgw: check placement existence when create bucket
Signed-off-by: Jiaying Ren <jiaying.ren@umcloud.com>
@mikulely

This comment has been minimized.

Contributor

mikulely commented Jul 19, 2017

Jenkins, retest this please.

1 similar comment
@mikulely

This comment has been minimized.

Contributor

mikulely commented Jul 19, 2017

Jenkins, retest this please.

@yuriw

This comment has been minimized.

@cbodley

This comment has been minimized.

Contributor

cbodley commented Jul 24, 2017

ping @joscollin

comment addressed, ready to merge

@cbodley cbodley merged commit dfd5af2 into ceph:master Jul 25, 2017

4 checks passed

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

@mikulely mikulely deleted the mikulely:3-check-target-when-create-bucket branch Jul 26, 2017

@joscollin

This comment has been minimized.

Member

joscollin commented Jul 26, 2017

@cbodley Sorry, I saw your ping right now, as I get a lot of emails everyday. Thank you for reviewing.

@mikulely

This comment has been minimized.

Contributor

mikulely commented Jul 26, 2017

Thanks @joscollin @cbodley @yuriw for merging this as always.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment