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 support container and object levels of swift bulkupload #14775

Merged
merged 1 commit into from May 1, 2017

Conversation

Projects
None yet
2 participants
@Jing-Scott
Contributor

Jing-Scott commented Apr 25, 2017

Signed-off-by: Jing Wenjun jingwenjun@cmss.chinamobile.com

@Jing-Scott

This comment has been minimized.

Contributor

Jing-Scott commented Apr 25, 2017

The blow is my local testing result.

The bulk-1.tar contains 123.txt and 2.txt.

  • bulkupload at container level
[root@CephS debugfile]# curl -i "${storageURL}/con1/?extract-archive=tar" -X PUT -H "X-Auth-Token: ${token}"  -T bulk-1.tar 
HTTP/1.1 100 CONTINUE

HTTP/1.1 200 OK
Transfer-Encoding: chunked
X-Trans-Id: tx00000000000000000003a-0058ff5734-1018-default
Content-Type: text/plain; charset=utf-8
Date: Tue, 25 Apr 2017 14:03:33 GMT

Number Files Created: 2
Response Body: 
Response Status: 201 Created
Errors: 

[root@CephS debugfile]# curl -H "X-Auth-Token: ${token}" "${storageURL}/con1"
bulk-1/123.txt
bulk-1/2.txt
  • bulkupload at object level
[root@CephS debugfile]# curl -i "${storageURL}/con2/file1/?extract-archive=tar" -X PUT -H "X-Auth-Token: ${token}"  -T bulk-1.tar 
HTTP/1.1 100 CONTINUE

HTTP/1.1 200 OK
Transfer-Encoding: chunked
X-Trans-Id: tx00000000000000000003b-0058ff575a-1018-default
Content-Type: text/plain; charset=utf-8
Date: Tue, 25 Apr 2017 14:04:10 GMT

Number Files Created: 2
Response Body: 
Response Status: 201 Created
Errors: 

[root@CephS debugfile]# curl -H "X-Auth-Token: ${token}" "${storageURL}/con2"
file1/bulk-1/123.txt
file1/bulk-1/2.txt
@Jing-Scott

This comment has been minimized.

Contributor

Jing-Scott commented Apr 25, 2017

Hi, @rzarzynski can you help to have a look? thanks in advance!

@rzarzynski rzarzynski self-assigned this Apr 25, 2017

@rzarzynski

@Jing-Scott: big thanks for bringing support for this missing part of BulkUpload! The code looks good. Only some cosmetics left. Please see my comments.

@@ -5933,6 +5933,19 @@ void RGWBulkUploadOp::execute()
return;
}
std::string bucket_path, file_prefix;
if (! s->init_state.url_bucket.empty()) {

This comment has been minimized.

@rzarzynski

rzarzynski Apr 26, 2017

Contributor

May we have this block dissected into a separated function/method? Then, we could write here something like that:

  /* Handling the $UPLOAD_PATH accordingly to the Swift's Bulk middleware. See: 
   * https://github.com/openstack/swift/blob/2.13.0/swift/common/middleware/bulk.py#L31-L41 */
  std::string bucket_path, file_prefix;
  std::tie(bucket_path, file_prefix) = handle_upload_path(s);

This comment has been minimized.

@Jing-Scott

Jing-Scott Apr 27, 2017

Contributor

a separated method makes sense!

file_prefix = bucket_path = s->init_state.url_bucket + "/";
if (!s->object.empty()) {
std::string& object_name = s->object.name;
if (object_name[object_name.length() -1] == '/') {

This comment has been minimized.

@rzarzynski

rzarzynski Apr 26, 2017

Contributor

C++11 has brought std::string::back() which will be useful here.

This comment has been minimized.

@Jing-Scott

Jing-Scott Apr 27, 2017

Contributor

yeah

std::string bucket_path, file_prefix;
if (! s->init_state.url_bucket.empty()) {
file_prefix = bucket_path = s->init_state.url_bucket + "/";
if (!s->object.empty()) {

This comment has been minimized.

@rzarzynski

rzarzynski Apr 26, 2017

Contributor

OK, rgw_obj_key::empty checks whether its name is empty, so we can safely examine the last element of object_name.

file_prefix = bucket_path = s->init_state.url_bucket + "/";
if (!s->object.empty()) {
std::string& object_name = s->object.name;
if (object_name[object_name.length() -1] == '/') {

This comment has been minimized.

@rzarzynski

rzarzynski Apr 26, 2017

Contributor

It would be nice to have a comment before the if:

      /* As rgw_obj_key::empty() already verified emptiness of s->object.name,
       * we can safely examine its last element. */
@Jing-Scott

This comment has been minimized.

Contributor

Jing-Scott commented Apr 27, 2017

Hi, @rzarzynski Thanks again for your review. Your comments are very helpful for me! Now I've updated them.

@rzarzynski

Thanks for the update! We're going in the right direction. See my comments.

@@ -5559,6 +5559,28 @@ RGWBulkUploadOp::parse_path(const boost::string_ref& path)
return boost::none;
}
boost::optional<std::pair<std::string, std::string>>

This comment has been minimized.

@rzarzynski

rzarzynski Apr 27, 2017

Contributor

I don't think we really need the boost::optional here especially are as we're dereferencing it without the check for emptiness.

}
return std::make_pair(bucket_path, file_prefix);
}
return boost::none;

This comment has been minimized.

@rzarzynski

rzarzynski Apr 27, 2017

Contributor

Maybe just:

  return std::make_pair(bucket_path, file_prefix)
/* Handling the $UPLOAD_PATH accordingly to the Swift's Bulk middleware. See:
* https://github.com/openstack/swift/blob/2.13.0/swift/common/middleware/bulk.py#L31-L41 */
std::string bucket_path, file_prefix;
std::tie(bucket_path, file_prefix) = *handle_upload_path(s);

This comment has been minimized.

@rzarzynski

rzarzynski Apr 27, 2017

Contributor

Here is the above mentioned dereference.

rgw: add support container and object levels of swift bulkupload
Signed-off-by: Jing Wenjun <jingwenjun@cmss.chinamobile.com>
@Jing-Scott

This comment has been minimized.

Contributor

Jing-Scott commented Apr 27, 2017

@rzarzynski done! thanks:-)

@rzarzynski

@Jing-Scott: LGTM, thanks! I'm taking this into local testing.

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented Apr 27, 2017

jenkins test this please

1 similar comment
@Jing-Scott

This comment has been minimized.

Contributor

Jing-Scott commented Apr 28, 2017

jenkins test this please

@rzarzynski

This comment has been minimized.

Contributor

rzarzynski commented May 1, 2017

This branch has been tested in following Teuthology runs:

The results look good. The failures have been caused by the well-known strange leak of std::string memory from md_config_t seen in radosgw signalised by Valgrind.

Also Tempest confirms the BulkUpload works:

$ ./run_tempest.sh -V tempest.api.object_storage.test_account_bulk.BulkTest
WARNING: This script is deprecated and will be removed in the near future. Please migrate to tempest run or another method of launching a test runner
running=OS_STDOUT_CAPTURE=${OS_STDOUT_CAPTURE:-1} \
OS_STDERR_CAPTURE=${OS_STDERR_CAPTURE:-1} \
OS_TEST_TIMEOUT=${OS_TEST_TIMEOUT:-500} \
OS_TEST_LOCK_PATH=${OS_TEST_LOCK_PATH:-${TMPDIR:-'/tmp'}} \
${PYTHON:-python} -m subunit.run discover -t ${OS_TOP_LEVEL:-./} ${OS_TEST_PATH:-./tempest/test_discover} --list 
running=OS_STDOUT_CAPTURE=${OS_STDOUT_CAPTURE:-1} \
OS_STDERR_CAPTURE=${OS_STDERR_CAPTURE:-1} \
OS_TEST_TIMEOUT=${OS_TEST_TIMEOUT:-500} \
OS_TEST_LOCK_PATH=${OS_TEST_LOCK_PATH:-${TMPDIR:-'/tmp'}} \
${PYTHON:-python} -m subunit.run discover -t ${OS_TOP_LEVEL:-./} ${OS_TEST_PATH:-./tempest/test_discover}  --load-list /tmp/tmpzPN4Vp
{0} tempest.api.object_storage.test_account_bulk.BulkTest.test_bulk_delete [0.217015s] ... ok
{0} tempest.api.object_storage.test_account_bulk.BulkTest.test_bulk_delete_by_POST [0.242554s] ... ok
{0} tempest.api.object_storage.test_account_bulk.BulkTest.test_extract_archive [0.083343s] ... ok

======
Totals
======
Ran: 3 tests in 3.0000 sec.
 - Passed: 3
 - Skipped: 0
 - Expected Fail: 0
 - Unexpected Success: 0
 - Failed: 0
Sum of execute time for each test: 0.5429 sec.

==============
Worker Balance
==============
 - Worker 0 (3 tests) => 0:00:00.544830

Thanks for bringing the feature, @Jing-Scott! Merging.

@rzarzynski rzarzynski merged commit 56baf95 into ceph:master May 1, 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

@Jing-Scott Jing-Scott deleted the Jing-Scott:wip-con-obj-bulkupload branch May 2, 2017

@Jing-Scott

This comment has been minimized.

Contributor

Jing-Scott commented May 2, 2017

thanks again for your review @rzarzynski :-)

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