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

Support for internal endpoints in cloudfiles #229

Closed
wants to merge 1 commit into from

Conversation

@jobelenus
Copy link
Contributor

@jobelenus jobelenus commented Jan 24, 2014

Sorry for the ugliness of the commit history. I didn't do this cleanly from the start. The diff is clean, however.

Being able to use the internal network endpoint gives significant speed improvements (upwards of a whole second from my brief benchmarking) when your application server and cloudfiles are in the same region/datacenter.

RE: #186 (comment)

@@ -112,6 +112,7 @@ def __init__(self, user_id, key, secure=True, **kwargs):
self.api_version = API_VERSION
self.accept_format = 'application/json'
self.cdn_request = False
self.endpoint_url = 'internalURL' if 'use_internal_url' in kwargs else 'publicURL'

This comment has been minimized.

@alex

alex Jan 24, 2014
Contributor

Passing use_internal_url=False shouldn't result in using it.

This comment has been minimized.

@Kami

Kami Jan 24, 2014
Member

Correct.

Please also explicitly declare use_internal_url argument in the method signature instead of "abusing" kwargs.

@Kami
Copy link
Member

@Kami Kami commented Jan 24, 2014

Adding a test case for this new functionality would also be good.

@jobelenus
Copy link
Contributor Author

@jobelenus jobelenus commented Jan 24, 2014

A test case would be interesting, since the internal endpoint is only accessible from within the datacenter network. Is the test runner within that network?

@Kami
Copy link
Member

@Kami Kami commented Jan 24, 2014

@jobelenus Sorry, I didn't actually meant doing the whole integration test. Sadly this would be too painful.

I was just thinking of adding a simple unit test which tests that get_endpoint() method returns a correct value depending on the use_internal_url argument.

@jobelenus
Copy link
Contributor Author

@jobelenus jobelenus commented Jan 24, 2014

If I've correctly understood how you're doing the Driver/Connection kwargs, this testcase should meet your requirements. I may be wrong though

@Kami
Copy link
Member

@Kami Kami commented Jan 24, 2014

@jobelenus Yeah, it's close enough :)

@Kami
Copy link
Member

@Kami Kami commented Jan 24, 2014

Can you please fix lint (https://travis-ci.org/apache/libcloud/builds/17566981) and squash all commits?

Once this is done, I'll go ahead and merge patch into trunk.

@jobelenus
Copy link
Contributor Author

@jobelenus jobelenus commented Jan 24, 2014

Yea... that doesn't make sense from Travis. Over-indent then under-indent when I take away a tab?

@Kami
Copy link
Member

@Kami Kami commented Jan 24, 2014

@jobelenus Do you happen to use tabs? If so, you should really use spaces.

Looking at the file, you are missing one whitespace on line 110.

@jobelenus
Copy link
Contributor Author

@jobelenus jobelenus commented Jan 24, 2014

I do use tabs. I don't see a missing whitespace? my line 110 matches line 147 (which is untouched) in behavior.

@Kami
Copy link
Member

@Kami Kami commented Jan 24, 2014

It should match with a first argument, but currently it's matching with an open parenthesis.

selection_028

@jobelenus
Copy link
Contributor Author

@jobelenus jobelenus commented Jan 24, 2014

Ah. I didn't understand the Travis lint was expecting it to match to the first char of the first arg. Thank you.

@Kami
Copy link
Member

@Kami Kami commented Jan 24, 2014

Np. Next time autopep8 also might or might not (it might mess up the existing style) help :)

@Kami
Copy link
Member

@Kami Kami commented Jan 24, 2014

Also, please ping me when the commits are squashed.

re-working `use_internal_url` in declaration, adding test case for endpoint variable

lint white-space fix

lint white-space fix

match indent to the first character of first arg
@jobelenus
Copy link
Contributor Author

@jobelenus jobelenus commented Jan 24, 2014

Squashed up. Wow that was ugly. Shame on me for not being clean in the first place.

@Kami
Copy link
Member

@Kami Kami commented Jan 24, 2014

Patch merged into trunk. This pull request can be closed now.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.