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

jewel: rgw: bucket index check in radosgw-admin removes valid index #16856

Merged
merged 1 commit into from Sep 12, 2017

Conversation

Projects
None yet
6 participants
@prallabh
Contributor

prallabh commented Aug 7, 2017

@prallabh

This comment has been minimized.

Show comment
Hide comment
@prallabh
Contributor

prallabh commented Aug 7, 2017

@smithfarm smithfarm added this to the jewel milestone Aug 7, 2017

@mattbenjamin mattbenjamin self-assigned this Aug 9, 2017

@mattbenjamin mattbenjamin requested a review from oritwas Aug 9, 2017

@oritwas

This comment has been minimized.

Show comment
Hide comment
@oritwas

oritwas Aug 10, 2017

Contributor

@prallabh , I don't see all the changes to check_bad_index_multipart (in the loop).
we still need:
string oid = obj.get_oid();

instead of:
string oid = key.name;

Contributor

oritwas commented Aug 10, 2017

@prallabh , I don't see all the changes to check_bad_index_multipart (in the loop).
we still need:
string oid = obj.get_oid();

instead of:
string oid = key.name;

@smithfarm smithfarm changed the title from jewel: rgw: bucket index check in radosgw-admin removes valid index. to jewel: rgw: bucket index check in radosgw-admin removes valid index Aug 10, 2017

@prallabh

This comment has been minimized.

Show comment
Hide comment
@prallabh

prallabh Aug 10, 2017

Contributor

@oritwas but ‘class rgw_obj’ has no member named ‘get_oid’ in Jewel, am I missing anything here?

Contributor

prallabh commented Aug 10, 2017

@oritwas but ‘class rgw_obj’ has no member named ‘get_oid’ in Jewel, am I missing anything here?

@oritwas

This comment has been minimized.

Show comment
Hide comment
@oritwas

oritwas Aug 10, 2017

Contributor

Correct we don't have it in Jewel but we need it because key.name won't work.
we will need to backport get_oid to Jewel (rgw_obj::get_oid and rgw_obj_key::get_oid).

Contributor

oritwas commented Aug 10, 2017

Correct we don't have it in Jewel but we need it because key.name won't work.
we will need to backport get_oid to Jewel (rgw_obj::get_oid and rgw_obj_key::get_oid).

rgw: bucket index check in radosgw-admin removes valid index.
Signed-off-by: Zhang Shaowen <zhangshaowen@cmss.chinamobile.com>
(cherry picked from commit a786252)
Signed-off-by: Pavan Rallabhandi <PRallabhandi@walmartlabs.com>

Conflicts:
	src/rgw/rgw_bucket.cc
		Jewel has RGWObjEnt, honor the same while populating obj oid
@prallabh

This comment has been minimized.

Show comment
Hide comment
@prallabh

prallabh Aug 10, 2017

Contributor

@oritwas do you want this commit to be backported 50c522e

Contributor

prallabh commented Aug 10, 2017

@oritwas do you want this commit to be backported 50c522e

@oritwas

This comment has been minimized.

Show comment
Hide comment
@oritwas

oritwas Aug 10, 2017

Contributor

@prallabh, we cannot add the commit that added it, it is too big for jewel.
It also introduced several regression which will require additional backports.
This will need to be a manual backport.

Contributor

oritwas commented Aug 10, 2017

@prallabh, we cannot add the commit that added it, it is too big for jewel.
It also introduced several regression which will require additional backports.
This will need to be a manual backport.

@smithfarm smithfarm changed the title from jewel: rgw: bucket index check in radosgw-admin removes valid index to [DNM] jewel: rgw: bucket index check in radosgw-admin removes valid index Aug 10, 2017

@prallabh

This comment has been minimized.

Show comment
Hide comment
@prallabh

prallabh Aug 11, 2017

Contributor

@oritwas while trying to look at get_oid(), am having to pull in extra fields like string ns and others. Am afraid I do not have full understanding of these structures and additional changes that might be required elsewhere, which would otherwise potentially introduce new regressions. Can you please take this over, have a feel this would be much faster for you to do it, than review my half baked changes. Please let me know if you want to close this PR out and have a fresh one out, thanks!

Contributor

prallabh commented Aug 11, 2017

@oritwas while trying to look at get_oid(), am having to pull in extra fields like string ns and others. Am afraid I do not have full understanding of these structures and additional changes that might be required elsewhere, which would otherwise potentially introduce new regressions. Can you please take this over, have a feel this would be much faster for you to do it, than review my half baked changes. Please let me know if you want to close this PR out and have a fresh one out, thanks!

@oritwas

This comment has been minimized.

Show comment
Hide comment
@oritwas

oritwas Aug 11, 2017

Contributor

@prallabh sure :)

Contributor

oritwas commented Aug 11, 2017

@prallabh sure :)

@@ -2131,7 +2132,7 @@ class RGWBucketMetadataHandler : public RGWMetadataHandler {
return ret;
/*
* We're unlinking the bucket but we don't want to update the entrypoint here we're removing
* We're unlinking the bucket but we don't want to update the entrypoint here - we're removing

This comment has been minimized.

@amitkumar50

amitkumar50 Aug 14, 2017

Contributor

@danderson As per coding guidelines, Instead of short forms we should use complete words
we're => We are
don't => do not
Kindly have a look at PR: #16705

@amitkumar50

amitkumar50 Aug 14, 2017

Contributor

@danderson As per coding guidelines, Instead of short forms we should use complete words
we're => We are
don't => do not
Kindly have a look at PR: #16705

This comment has been minimized.

@amitkumar50

amitkumar50 Sep 1, 2017

Contributor

ignore it plz

@amitkumar50

amitkumar50 Sep 1, 2017

Contributor

ignore it plz

@amitkumar50

Please see comments inline

@oritwas

This comment has been minimized.

Show comment
Hide comment
@oritwas

oritwas Aug 15, 2017

Contributor

@amitkumar50 , this is a backport. It cannot contain changes that are not in master.
The changes you requested should be fixed in master not here.

Contributor

oritwas commented Aug 15, 2017

@amitkumar50 , this is a backport. It cannot contain changes that are not in master.
The changes you requested should be fixed in master not here.

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Aug 29, 2017

Contributor

@oritwas Status update?

Contributor

smithfarm commented Aug 29, 2017

@oritwas Status update?

@@ -2131,7 +2132,7 @@ class RGWBucketMetadataHandler : public RGWMetadataHandler {
return ret;
/*
* We're unlinking the bucket but we don't want to update the entrypoint here we're removing
* We're unlinking the bucket but we don't want to update the entrypoint here - we're removing

This comment has been minimized.

@amitkumar50

amitkumar50 Sep 1, 2017

Contributor

ignore it plz

@amitkumar50

amitkumar50 Sep 1, 2017

Contributor

ignore it plz

@@ -1034,6 +1034,7 @@ int RGWBucket::check_bad_index_multipart(RGWBucketAdminOpState& op_state,
RGWRados::Bucket::List list_op(&target);
list_op.params.list_versions = true;
list_op.params.ns = ns;

This comment has been minimized.

@amitkumar50

amitkumar50 Sep 1, 2017

Contributor

In recent master,
#define RGW_OBJ_NS_MULTIPART "multipart"
list_op.params.ns = RGW_OBJ_NS_MULTIPART;

Macro is taken instead of local declaration.
This PR can be dropped.

@amitkumar50

amitkumar50 Sep 1, 2017

Contributor

In recent master,
#define RGW_OBJ_NS_MULTIPART "multipart"
list_op.params.ns = RGW_OBJ_NS_MULTIPART;

Macro is taken instead of local declaration.
This PR can be dropped.

@oritwas oritwas self-assigned this Sep 4, 2017

@oritwas

This comment has been minimized.

Show comment
Hide comment
@oritwas

oritwas Sep 12, 2017

Contributor

After a second review this should work for Jewel without any more changes, approving

Contributor

oritwas commented Sep 12, 2017

After a second review this should work for Jewel without any more changes, approving

@oritwas oritwas changed the title from [DNM] jewel: rgw: bucket index check in radosgw-admin removes valid index to jewel: rgw: bucket index check in radosgw-admin removes valid index Sep 12, 2017

@oritwas

This comment has been minimized.

Show comment
Hide comment
@oritwas

oritwas Sep 12, 2017

Contributor

@smithfarm , this looks good to go

Contributor

oritwas commented Sep 12, 2017

@smithfarm , this looks good to go

@prallabh

This comment has been minimized.

Show comment
Hide comment
@prallabh

prallabh Sep 12, 2017

Contributor

@smithfarm now that it's been approved, it would be great to have this included in 10.2.10. Please let me know if that's possible.

Contributor

prallabh commented Sep 12, 2017

@smithfarm now that it's been approved, it would be great to have this included in 10.2.10. Please let me know if that's possible.

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 12, 2017

Contributor

I am running another integration branch and this PR is included.

Contributor

smithfarm commented Sep 12, 2017

I am running another integration branch and this PR is included.

@smithfarm

This comment has been minimized.

Show comment
Hide comment
@smithfarm

smithfarm Sep 12, 2017

Contributor

This passed an rgw suite at http://tracker.ceph.com/issues/20613#note-63

Contributor

smithfarm commented Sep 12, 2017

This passed an rgw suite at http://tracker.ceph.com/issues/20613#note-63

@smithfarm smithfarm merged commit bd918be into ceph:jewel Sep 12, 2017

3 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment