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

Support for EMC Atmos and thus Ninefold Storage #19

Closed
wants to merge 39 commits into
base: trunk
from

Conversation

Projects
None yet
2 participants
@jeamland

jeamland commented Jul 3, 2011

These commits add support for the EMC Atmos storage API and add Ninefold as a storage provider.

xhdrs = [(k, v) for k, v in headers.items() if k.startswith('x-emc-')]
xhdrs.sort(key=lambda x: x[0])
signature = [

This comment has been minimized.

@Kami

Kami Jul 3, 2011

Member

It's probably a good idea to refactor signature part into a separate method. It will also make testing easier.

@Kami

Kami Jul 3, 2011

Member

It's probably a good idea to refactor signature part into a separate method. It will also make testing easier.

This comment has been minimized.

@jeamland

jeamland Jul 4, 2011

Signature method split out in 6d68c6b, tests for signature method added in 488965b.

@jeamland

jeamland Jul 4, 2011

Signature method split out in 6d68c6b, tests for signature method added in 488965b.

if pathstring.startswith(self.driver.path):
pathstring = pathstring[len(self.driver.path):]
if params:
if type(params) is dict:

This comment has been minimized.

@Kami

Kami Jul 7, 2011

Member

Hm, I don't really like this check...

Maybe we should just thrown a TypeError if users passes in anything other than dict as the params argument.

@Kami

Kami Jul 7, 2011

Member

Hm, I don't really like this check...

Maybe we should just thrown a TypeError if users passes in anything other than dict as the params argument.

This comment has been minimized.

@jeamland

jeamland Jul 16, 2011

urllib allows either a dict or an iterable of 2-tuples. The check is to force it to the latter so that the order remains the same. I'm happy to code in an assumption that it's dict but it depends on whether you want to impose that constraint.

@jeamland

jeamland Jul 16, 2011

urllib allows either a dict or an iterable of 2-tuples. The check is to force it to the latter so that the order remains the same. I'm happy to code in an assumption that it's dict but it depends on whether you want to impose that constraint.

Show outdated Hide outdated libcloud/storage/drivers/atmos.py
try:
self.connection.request(request_path + '?metadata/system')
except Exception, e:

This comment has been minimized.

@Kami

Kami Jul 7, 2011

Member

Hm, you do a similar check in multiple places.

You should probably override the request method and handle this there so we can only do it once?

Method signature could for example look something like this: def request(self, path, expected_codes) ...

@Kami

Kami Jul 7, 2011

Member

Hm, you do a similar check in multiple places.

You should probably override the request method and handle this there so we can only do it once?

Method signature could for example look something like this: def request(self, path, expected_codes) ...

This comment has been minimized.

@jeamland

jeamland Jul 16, 2011

Well the issue is more that when an Exception is raised, I'm looking for specific codes. In abe318b I've changed it somewhat to make it a bit less brittle but I can't think of an obvious way to make it any better abstracted than that.

@jeamland

jeamland Jul 16, 2011

Well the issue is more that when an Exception is raised, I'm looking for specific codes. In abe318b I've changed it somewhat to make it a bit less brittle but I can't think of an obvious way to make it any better abstracted than that.

Show outdated Hide outdated libcloud/storage/drivers/atmos.py
response = result_dict['response'].response
bytes_transferred = result_dict['bytes_transferred']
range_hdr = str(bytes_transferred) + '-' + str(bytes_transferred)

This comment has been minimized.

@Kami

Kami Jul 7, 2011

Member

Hm, this variable is defined but unused?

@Kami

Kami Jul 7, 2011

Member

Hm, this variable is defined but unused?

This comment has been minimized.

@jeamland

jeamland Jul 16, 2011

Yep, gone in 0628edd.

@jeamland
meta = self._emc_meta(result)
del meta_data['md5']
extra = {
'object_id': meta['objectid']

This comment has been minimized.

@Kami

Kami Jul 7, 2011

Member

You never add meta_data dictionary to the extra dictionary?

@Kami

Kami Jul 7, 2011

Member

You never add meta_data dictionary to the extra dictionary?

This comment has been minimized.

@jeamland
@jeamland
Show outdated Hide outdated libcloud/storage/drivers/atmos.py
data_hash = hashlib.md5()
def chunkify(source):

This comment has been minimized.

@Kami

Kami Jul 7, 2011

Member

There already is a read_in_chunks utility function in libcloud/utils.py. We should modify it to support this use case instead of doing this here.

@Kami

Kami Jul 7, 2011

Member

There already is a read_in_chunks utility function in libcloud/utils.py. We should modify it to support this use case instead of doing this here.

This comment has been minimized.

@jeamland

jeamland Jul 16, 2011

Done in 82d710e. Also added tests for read_in_chunks.

@jeamland

jeamland Jul 16, 2011

Done in 82d710e. Also added tests for read_in_chunks.

}
if len(chunk) > 0:
headers['Range'] = 'Bytes=%d-%d' % (bytes_transferred, end)
result = self.connection.request(path, method='PUT', data=chunk,

This comment has been minimized.

@Kami

Kami Jul 7, 2011

Member

So Atmos doesn't support chunked encoding?

@Kami

Kami Jul 7, 2011

Member

So Atmos doesn't support chunked encoding?

This comment has been minimized.

@jeamland
@jeamland
end = bytes_transferred + len(chunk) - 1
data_hash.update(chunk)
headers = {
'x-emc-meta': 'md5=' + data_hash.hexdigest(),

This comment has been minimized.

@Kami

Kami Jul 7, 2011

Member

Hm, is this correct? Shouldn't you only send md5 hash for the current chunk and not for all the chunks which have been transfered?

@Kami

Kami Jul 7, 2011

Member

Hm, is this correct? Shouldn't you only send md5 hash for the current chunk and not for all the chunks which have been transfered?

This comment has been minimized.

@jeamland

jeamland Jul 16, 2011

It's a range request. I'm basically updating the md5 for what's up there.

@jeamland

jeamland Jul 16, 2011

It's a range request. I'm basically updating the md5 for what's up there.

This comment has been minimized.

@Kami

Kami Jul 17, 2011

Member

Ah, missed that.

@Kami

Kami Jul 17, 2011

Member

Ah, missed that.

Show outdated Hide outdated libcloud/storage/drivers/atmos.py
def list_containers(self):
result = self.connection.request(self._namespace_path(''))
entries = self._list_objects(result.object)

This comment has been minimized.

@Kami

Kami Jul 7, 2011

Member

Maybe you should add type argument (directory / object / both) to the _list_objects method and perform filtering there?

@Kami

Kami Jul 7, 2011

Member

Maybe you should add type argument (directory / object / both) to the _list_objects method and perform filtering there?

This comment has been minimized.

@jeamland
@jeamland
Show outdated Hide outdated libcloud/storage/drivers/atmos.py
}
objects.append(Object(entry['name'], 0, '', {}, metadata, container,
self))
return objects

This comment has been minimized.

@Kami

Kami Jul 7, 2011

Member

To be consistent with other drivers, this method should return a LazyList object. For example you can look at the S3 / CloudFiles driver - https://github.com/apache/libcloud/blob/trunk/libcloud/storage/drivers/s3.py#L178

@Kami

Kami Jul 7, 2011

Member

To be consistent with other drivers, this method should return a LazyList object. For example you can look at the S3 / CloudFiles driver - https://github.com/apache/libcloud/blob/trunk/libcloud/storage/drivers/s3.py#L178

This comment has been minimized.

@jeamland
@jeamland
Show outdated Hide outdated libcloud/storage/drivers/atmos.py
]
key = base64.b64decode(self.secret)
signature = '\n'.join(['GET', path.lower(), self.key, expiry])

This comment has been minimized.

@Kami

Kami Jul 7, 2011

Member

We should move signature calculation to a separate method.

@Kami

Kami Jul 7, 2011

Member

We should move signature calculation to a separate method.

This comment has been minimized.

@jeamland
@jeamland
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 15, 2011

Member

Any updates? It would be nice to merge it.

Member

Kami commented Jul 15, 2011

Any updates? It would be nice to merge it.

@jeamland

This comment has been minimized.

Show comment
Hide comment
@jeamland

jeamland Jul 16, 2011

Sorry about the delay. Our second child arrived on Monday and strangely I haven't had much time to hack on things. =)

jeamland commented Jul 16, 2011

Sorry about the delay. Our second child arrived on Monday and strangely I haven't had much time to hack on things. =)

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 17, 2011

Member

No problem and congratulations!

I will try to merge this branch over the weekend :)

Member

Kami commented Jul 17, 2011

No problem and congratulations!

I will try to merge this branch over the weekend :)

raise StopIteration
yield chunk
if fill_size:
if empty or len(data) >= chunk_size:

This comment has been minimized.

@Kami

Kami Jul 17, 2011

Member

Equality comparison here doesn't really make sense because it's the same as the else case.

@Kami

Kami Jul 17, 2011

Member

Equality comparison here doesn't really make sense because it's the same as the else case.

This comment has been minimized.

@jeamland

jeamland Jul 20, 2011

The else case is against if fill_size though. Basically that test doesn't bother sending a chunk unless it's the right size or unless we've run out of new data.

@jeamland

jeamland Jul 20, 2011

The else case is against if fill_size though. Basically that test doesn't bother sending a chunk unless it's the right size or unless we've run out of new data.

This comment has been minimized.

@Kami

Kami Jul 20, 2011

Member

Ah, oops, I missed that.

@Kami

Kami Jul 20, 2011

Member

Ah, oops, I missed that.

Show outdated Hide outdated test/test_utils.py
fill_size=False):
self.assertEqual(result, 'bbbbbbbbbb')
for result in libcloud.utils.read_in_chunks(FakeFile(), chunk_size=10,

This comment has been minimized.

@Kami

Kami Jul 17, 2011

Member

This doesn't really test that the fill_size works properly.

Something like this should work better:

    def test_read_in_chunks_filelike(self):
        class FakeFile(file):
            def __init__(self):
                self.remaining = 500

            def read(self, size):
                self.remaining -= 1
                if self.remaining == 0:
                    return ''
                return 'b' * (size + 1)

        for index, result in enumerate(libcloud.utils.read_in_chunks(
                                       FakeFile(), chunk_size=10,
                                       fill_size=False)):
            self.assertEqual(result, 'b' * 11)

        self.assertEqual(index, 498)

        for index, result in enumerate(libcloud.utils.read_in_chunks(
                                       FakeFile(), chunk_size=10,
                                       fill_size=True)):
            if index != 548:
                self.assertEqual(result, 'b' * 10)
            else:
                self.assertEqual(result, 'b' * 9)

        self.assertEqual(index, 548)
@Kami

Kami Jul 17, 2011

Member

This doesn't really test that the fill_size works properly.

Something like this should work better:

    def test_read_in_chunks_filelike(self):
        class FakeFile(file):
            def __init__(self):
                self.remaining = 500

            def read(self, size):
                self.remaining -= 1
                if self.remaining == 0:
                    return ''
                return 'b' * (size + 1)

        for index, result in enumerate(libcloud.utils.read_in_chunks(
                                       FakeFile(), chunk_size=10,
                                       fill_size=False)):
            self.assertEqual(result, 'b' * 11)

        self.assertEqual(index, 498)

        for index, result in enumerate(libcloud.utils.read_in_chunks(
                                       FakeFile(), chunk_size=10,
                                       fill_size=True)):
            if index != 548:
                self.assertEqual(result, 'b' * 10)
            else:
                self.assertEqual(result, 'b' * 9)

        self.assertEqual(index, 548)

This comment has been minimized.

@jeamland
@jeamland
@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Jul 21, 2011

Member

Looks OK. Can you please create a ticket on jira and submit a patch there and I'll try to merge it today?

Thanks!

Member

Kami commented Jul 21, 2011

Looks OK. Can you please create a ticket on jira and submit a patch there and I'll try to merge it today?

Thanks!

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Oct 26, 2013

Member

(just doing some pull request cleanup)

@jeamland - Those changes have been merged a long time ago, can you please close this pull request?

Thanks!

Member

Kami commented Oct 26, 2013

(just doing some pull request cleanup)

@jeamland - Those changes have been merged a long time ago, can you please close this pull request?

Thanks!

@asfgit asfgit closed this in 5591e2e Jan 17, 2015

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