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

Optimization #16

Open
klinkin opened this issue Jan 23, 2014 · 6 comments
Open

Optimization #16

klinkin opened this issue Jan 23, 2014 · 6 comments

Comments

@klinkin
Copy link

klinkin commented Jan 23, 2014

If bucket already exist and contains thousands static files
https://github.com/e-dard/flask-s3/blob/master/flask_s3.py#L188

bucket.make_public(recursive=True)

it's very bad idea...

@e-dard
Copy link
Owner

e-dard commented Jan 23, 2014

Hmmm, a good point.

I don't have a lot of time right now but I guess a solution could involves using get_all_buckets() and checking for the bucket in question. Details here

@klinkin
Copy link
Author

klinkin commented Jan 27, 2014

i'm working on it just now.

klinkin added a commit to klinkin/flask-s3 that referenced this issue Jan 28, 2014
Trim EOL whitespace and other PEP8 issue by autopep8
Use tqdm for progress bar if log level=INFO
@eriktaubeneck
Copy link
Collaborator

I'm not entirely sure that simply changing recursive=False as suggested in the PR will do the trick. If a bucket already exists, it is likely that you'd want to make the entire thing public. @e-dard, if we checked get_all_buckets(), would be not make it public recursively? Whatever we do, there should be something added to the documentation to explain that a user needs to make the existing stuff public.

I'm also not entirely sure why that section is in that try/except block. @e-dard maybe you can enlighten my, but effectively we are now ignoring all but 1 specific error. That's probably a bad idea, and if we are just going to raise that error anyways, why not remove the try/except entirely?

@klinkin
Copy link
Author

klinkin commented Jan 30, 2014

  1. Recursive mode
    1.1 If we creat new bucket then it's empty and there is no need to do make_public in recursive mode.
    1.2 If target bucket already exists then with high probability it’s already public. And all static files makes public when it copied, see https://github.com/e-dard/flask-s3/blob/master/flask_s3.py#L113
  2. Try/except block
    In master branch is the same implementation. See https://github.com/e-dard/flask-s3/blob/master/flask_s3.py#L186

@eriktaubeneck
Copy link
Collaborator

  1. It could be the case that there are files in S3 that aren't copied, but this is an edge case. I still think we should document it. 
  2. Yes, but your change brought it to my attention. ;)
    Sent from my Nintendo 64

On Thu, Jan 30, 2014 at 5:57 PM, Mike [email protected] wrote:

  1. Recursive mode
    1.1 If we creat new bucket then it's empty and there is no need to do make_public in recursive mode.
    1.2 If target bucket already exists then with high probability it’s already public. And all static files makes public when it copied, see https://github.com/e-dard/flask-s3/blob/master/flask_s3.py#L113
  2. Try/except block
    In master branch is the same implementation. See https://github.com/e-dard/flask-s3/blob/master/flask_s3.py#L186

    Reply to this email directly or view it on GitHub:
    Optimization #16 (comment)

@e-dard
Copy link
Owner

e-dard commented Feb 9, 2014

Yeah, oops on the try block 😊

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

Successfully merging a pull request may close this issue.

3 participants