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: avoid listing user buckets for rgw_delete_user #13991

Merged
merged 1 commit into from Mar 24, 2017

Conversation

Projects
None yet
5 participants
@Liuchang0812
Copy link
Contributor

Liuchang0812 commented Mar 16, 2017

@cbodley @yehudasa @mattbenjamin

Would you mind taking a look. Appreciated!

rgw: optimize rgw performance, avoid unnecessary bucket list and memo…
…ry copy

We do not use buckets_vec in following code, and rgw_read_user_buckets with
`need_stats=false` will not modify other variables. So we could remove this
bucket-listing code, get better performance.

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
first_data = true;
cur_ofs = 0;
RGWGetObj():
range_str(NULL),

This comment has been minimized.

Copy link
@theanalyst

theanalyst Mar 16, 2017

Member

NULL -> nullptr?

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Mar 16, 2017

Author Contributor

yep, nullptr is better, I will do it

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-rgw-optimization branch from 8a12973 to 3fe18d5 Mar 16, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Mar 16, 2017

Hi, all, I need your help, review this commit please: a465395

@@ -2160,8 +2160,6 @@ void RGWDeleteObj_ObjStore_S3::send_response()
{
int r = op_ret;
if (r == -ENOENT)
r = 0;
if (!r)

This comment has been minimized.

Copy link
@oritwas

oritwas Mar 16, 2017

Contributor

you need to keep this check, what if opt_ret is 0?

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Mar 16, 2017

Author Contributor

thanks, good catch! How about

int r = op_ret;
if (r == -ENOENT || r == 0) {
   r = STATUS_NO_CONTENT;
}

it's seems more readability.

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Mar 17, 2017

Author Contributor

I droped this commit, thanks @oritwas

q_ofs(0),
q_len(0),
first_data(true),
cur_ofs(0) {

This comment has been minimized.

Copy link
@cbodley

cbodley Mar 16, 2017

Contributor

i would prefer that we use inline initialization where these variables are declared, ie:

  const char *range_str{nullptr};
...
  uint32_t mod_zone_id{0};

this way, we only have to change one line when we add/remove members, and we can add extra constructors without duplicating all of them

(whether you use the var{foo} or var = foo syntax is up to you)

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Mar 16, 2017

Author Contributor

hi, @cbodley I want to make sure whether you suggest as following:

class Foo {
private:
  const char *bar{"helloworld"};
};

It was introduced since C++11, called class member initializers. IMO, I would like to do it in separated PR. This PR contains remove bucket listing in user delete only.

This comment has been minimized.

Copy link
@cbodley

cbodley Mar 16, 2017

Contributor

yep, that's exactly what i'm suggesting

IMO, I would like to do it in separated PR. This PR contains remove bucket listing in user delete only.

i'm confused, this a comment on your rgw/rgw_op: move initialization of class members to constructed function commit. i'd rather replace those assignments with the inline class member initializers

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Mar 17, 2017

Author Contributor

copy that

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Mar 17, 2017

Author Contributor

sorry, I mean that I will make a new PR that replace those assignments with the inline class member initializers. This PR #13991 will contains a465395 only.

@cbodley

This comment has been minimized.

Copy link
Contributor

cbodley commented Mar 16, 2017

Hi, all, I need your help, review this commit please: a465395

i'm not sure what that bucket listing in rgw_delete_user() was for, originally - do you know, @yehudasa?

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-rgw-optimization branch from 3fe18d5 to a465395 Mar 17, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Mar 20, 2017

change log:

  1. drop commit as @oritwas commeted.
  2. drop commits of class member initialization, I will open a new PR which contains those commits.

Now, here is only one commit a465395, Waiting for @yehudasa review.

@cbodley cbodley changed the title rgw: some trivial perfmance optimization rgw: avoid listing user buckets for rgw_delete_user Mar 23, 2017

@cbodley

This comment has been minimized.

@cbodley cbodley merged commit c899d7e into ceph:master Mar 24, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@Liuchang0812 Liuchang0812 deleted the Liuchang0812:wip-rgw-optimization branch Mar 24, 2017

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.