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

WIP: S3: Implementation for S3 Get Bucket Location #2329

Merged
merged 4 commits into from
Sep 5, 2014

Conversation

theanalyst
Copy link
Member

S3 API supports getting the location for a bucket, which gives out one
of those geographic zones (US-WEST-1, EU for eg). Also it returns an
empty string for the default region.
(http://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketGETlocation.html)

Since LocationConstraint corresponds to regions in our case, this API
returns the region, for the "default" region empty string is returned

Signed-off-by: Abhishek Lekshmanan abhishek.lekshmanan@ril.com

@theanalyst
Copy link
Member Author

@yehudasa thanks for the patient reviews so far! :) I have pushed the changes for XMLFormatter and JSON Formatter with a dump_format_ns.
(not sure of this) also I had to extend rgw_formats with the same function.. as of now I raised a NotImplementedError... should the implementation be done for this?

Also for get bucket location I have incorporated your comments changes. Let me know your comments

@yehudasa
Copy link
Member

@theanalyst Basically ok. There are a few issues though: json doesn't define a namespace (as far as I could fine at least), so the dump_format_ns() should just do what dump_format() does. The same goes for the plain formatter (which I might have missed, but didn't see its changes here).
The base Formatter::dump_format() should just call Formatter::dump_format_ns() with an empty string. Its specializations should be removed.
Also, you can probably squash all the formatter changes together.

@theanalyst
Copy link
Member Author

There are a few issues though: json doesn't define a namespace (as far as I could fine at least), so the dump_format_ns() should just do what dump_format() does.

Yeah I couldn't find anything abt json ns either.. just borrowed what json open section in ns does... I'll do what dump_format() does

The same goes for the plain formatter (which I might have missed, but didn't see its changes here).

The base Formatter::dump_format() should just call Formatter::dump_format_ns() with an empty string. Its specializations should be removed.

dump_format_ns with an empty ns should behave same as dump_format however, since this was a variadic fn and I cant pass on the varargs, I could write a helper function of sorts like which takes the namespace argument and either the buffer itself or a va_list and can be called by both? (there might be a short straight way to do this, do correct me if this is the case)

Also, you can probably squash all the formatter changes together.

Sure, will definitely do that.

@theanalyst
Copy link
Member Author

Hi @yehudasa I made dump_format_ns in json & plain formatter exactly same as dump_format. Squashed the formatter changes for json & XML into one. Also added a basic unit test for XML part of dump_format_ns. Let me know your comments

@yehudasa
Copy link
Member

yehudasa commented Sep 4, 2014

@theanalyst just squash db3554a and 94b8a19 (put them after the formatter commits).

`dump_format_ns` is a generic formatter to dump a simple format along
with a namespace. It is `dump_format` with an optional ns. This also
extends the XML formatter with this functionality. This allows creation
of xml tags with ns and a specified format. The JSON Format doesn't
define a ns, and here the functionality is exactly same as that of
`dump_format`

Signed-off-by: Abhishek Lekshmanan <abhishek.lekshmanan@ril.com>
Since the base formatter was extended with a dump_format_ns class,
implementing this here. For now, this is exactly same as dump_format, as
the concept of ns in json is not used.

Signed-off-by: Abhishek Lekshmanan <abhishek.lekshmanan@ril.com>
Adding basic unit test to test the new formatter class'
dump_format_ns. Since the functionality only affects XML (and other
implementations mimic dump_format exactly), tests are added for these.
`fmt.close_section()` is avoided in the tests as this calls an
assert (and there is no section to close) and this triggers a test
failure.

Signed-off-by: Abhishek Lekshmanan <abhishek.lekshmanan@ril.com>
S3 API supports getting the location for a bucket, which gives out one
of those geographic zones (US-WEST-1, EU for eg). Also it returns an
empty string for the default region.
(http://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketGETlocation.html)

Since LocationConstraint corresponds to regions in our case, this API
returns the region, for the "default" region empty string is returned

Signed-off-by: Abhishek Lekshmanan <abhishek.lekshmanan@ril.com>
@theanalyst
Copy link
Member Author

@yehudasa have squashed the commits onto d8e672f & rebased to move after formatter.

yehudasa added a commit that referenced this pull request Sep 5, 2014
WIP: S3: Implementation for S3 Get Bucket Location

Reviewed-by: Yehuda Sadeh <yehuda@redhat.com>
@yehudasa yehudasa merged commit 8c60286 into ceph:master Sep 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants