Skip to content
This repository has been archived by the owner on Sep 12, 2018. It is now read-only.

OPS-5907: remove ParallelKey class #961

Merged
merged 2 commits into from
Mar 10, 2015

Conversation

chaiken-verticloud
Copy link

This pull request removes the ParallelKey class as discussed in issue #956. Performance tests on AWS with an m3.medium docker client and different size registry servers (m3.large, m3.xlarge, c3.xlarge) indicate that:

  • the registry continues to work correctly
  • the performance of the registry with ParallelKey deleted is no worse than the performance of the registry with ParallelKey

There may be some use cases where ParallelKey helps performance, but I couldn't find them with the test configuration using the m3.medium client.

@@ -26,14 +26,8 @@

"""

import gevent.monkey
gevent.monkey.patch_all()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not take the chance to remove gevent patching here - though it seems ludicrous, I'm pretty sure there was a reason for it being duplicated here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I'll put it back in.

@dmp42
Copy link
Contributor

dmp42 commented Mar 8, 2015

One simple nit, LGTM otherwise.

@chaiken-verticloud
Copy link
Author

I restored the gevent.monkey.patch code, which causes the following flake8 errors:

depends/docker-registry-core/docker_registry/core/boto.py:32:1: E402 module level import not at top of file
depends/docker-registry-core/docker_registry/core/boto.py:33:1: E402 module level import not at top of file
depends/docker-registry-core/docker_registry/core/boto.py:35:1: E402 module level import not at top of file
depends/docker-registry-core/docker_registry/core/boto.py:36:1: E402 module level import not at top of file
depends/docker-registry-core/docker_registry/core/boto.py:37:1: E402 module level import not at top of file

flake8 generated similar errors before I modified the file.

I'll test the code with the restored gevent.monkey.patch (even thought I don't expect any problems from it).

@chaiken-verticloud
Copy link
Author

I tested the code with the restored gevent.monkey.patch with both [AWS_SECURE=false; STORAGE_REDIRECT=true] and [AWS_SECURE=true; STORAGE_REDIRECT=false]. Both worked as expected. Performance looks unchanged.

@dmp42
Copy link
Contributor

dmp42 commented Mar 10, 2015

In order to make flake/hacking stop complaining here, you can:

- gevent.monkey.patch_all()

+ gevent.monkey.patch_all() # noqa

LGTM otherwise - merging.

dmp42 added a commit that referenced this pull request Mar 10, 2015
@dmp42 dmp42 merged commit 3bb1dee into docker-archive:master Mar 10, 2015
@dmp42
Copy link
Contributor

dmp42 commented Mar 10, 2015

Fixed #956 and #954

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants