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

Library layer - first part #11

Merged
merged 27 commits into from
Jan 4, 2016
Merged

Conversation

ppolewicz
Copy link
Collaborator

This PR is WIP (created for easy early review). Please do not merge it before it is ready.

This will eventually implement #4.

Task list:

  • Create library access skeleton
  • Implement B2Api.list_buckets()
  • Implement B2Api.get_bucket_by_id()
  • Implement B2Api.create_bucket()
  • Implement B2Api.delete_bucket()
  • Implement B2Api.make_url()
  • Add api to bucket constructor
  • Implement AbstractBucket.hide_file()
  • drop AbstractBucket .getUploadUrl() (accidentally copied from java)
  • implement B2Api.delete_file_version()
  • implement bucket.set_type()
  • implement InMemoryCache.save_bucket()
  • remove FileInfo
  • implement clear_account (?)
  • rethink BucketType enum
  • rething AbstractBucket class hierarchy
  • implement api.download_file_by_id()
  • implement AbstractBucket.download_file_by_name()
  • implement bucket.list_file_names()
  • implement bucket.list_file_versions()
  • implement bucket.upload_file()
  • refactor "400 bad request" exception handling
  • remove ConsoleTool.info temporary property

Unfinished items will be done in another PR

@ppolewicz
Copy link
Collaborator Author

@bwbeach please review to confirm that our views are still aligned. list_buckets now has a library interface and a console command handler interface which depends on the library interface.

Console version should not be affected.

Example usage of library:

>>> import b2
>>> api = b2.B2Api(b2.StoredAccountInfo())
>>> for b in api.list_buckets():
...     print b.name
... 
test234
>>> b = api.get_bucket_by_name('foobar')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "b2.py", line 305, in get_bucket_by_name
    raise NonExistentBucket(bucket_name)
b2.NonExistentBucket: No such bucket: foobar
>>>

@ppolewicz
Copy link
Collaborator Author

@bwbeach this is blocking for me: I don't want to put a few dozen hours into it without first making sure that my way of doing it is fine for you (and will get merged). Could you please take a quick look at it by tomorrow EOD?

@bwbeach
Copy link
Contributor

bwbeach commented Dec 15, 2015

Pawel,

Sorry for the delay. Just got back traveling this weekend.

Your changes look good, and I like the direction you're taking. Just a few comments:

What's the reason for mapping bucket types to 1001 and 1002? It might be easier when debugging just to use the strings names internally.
I like the exception hierarchy.
I don't yet see where you're going with the class hierarchy AbstractBucket/UnknownTypeBucket/Bucket.
The layering is looking good.
Your coding style is clean and readable.

  • BrianB

@ppolewicz
Copy link
Collaborator Author

The thing with bucket types is a generic pattern for enum in python. You might be right, it may not be very useful here, I'll make a note to look at it again at the end and remove it if it is still not useful.

Bucket types is a defense against user having to check if fields of bucket are None everywhere, so that any operation on bucket is contained in the class rather than splattered in every place someone needs to use it... However it is clear that you are working with b2 much longer than me - I can not think of any operations that would need to be extracted like that. I added a note to review and simplify that as well.

Than you very much for the review, now I can proceed.

* origin/master:
  Add retry logic for uploads, to upload to a different storage pod if the chosen one was busy.
  Bump version number.
  Add comment about version number conventions.

Conflicts:
	b2
* origin/master:
  Bump version number.
  Check that file is not too big before trying to upload.
  Compute sha1 of uploaded file only once, even when retrying upload.
  Support external SHA1 sum

Conflicts:
	b2
@ppolewicz
Copy link
Collaborator Author

@bwbeach

api = b2.B2Api()
bucket = api.get_bucket_by_name('foobar')
api.delete_bucket(bucket)

or

api = b2.B2Api()
bucket = api.get_bucket_by_name('foobar')
bucket.delete()

? I think the first version is better, because bucket.delete() might be confused between deleting a file and deleting a bucket, while api.delete_bucket(bucket) can not be confused with anything.

@bwbeach
Copy link
Contributor

bwbeach commented Dec 26, 2015

api = b2.B2Api(b2.StoredAccountInfo())
bucket = api.get_bucket_by_name('foobar')
api.delete_bucket(bucket)
or

api = b2.B2Api(b2.StoredAccountInfo())
bucket = api.get_bucket_by_name('foobar')
bucket.delete()
? I think the first version is better, because bucket.delete() might be confused between deleting a file and deleting a bucket, while api.delete_bucket(bucket) can not be confused with anything.

I agree. I think api.delete_bucket(bucket) is clearer, and I like keeping the name "delete_bucket" consistent with the B2 web API.

  • BrianB

@ppolewicz
Copy link
Collaborator Author

@bwbeach why does b2_delete_file_version api call accept a file name parameter? Isn't the file id sufficient to identify a file in the b2 storage?

This is needed for bucket methods which need to call the API
* origin/master:
  Rename class stream_with_progress to StreamWithProgress, following PEP 8 conventions.
  Update shebang line to work with default OSX Python installation.
  Remove the option to disable progress bar during uploads
  Add a progress bar for downloads
  Add a progress bar for uploads
  Sort imports

Conflicts:
	b2
@bwbeach
Copy link
Contributor

bwbeach commented Dec 30, 2015

Passing the file name into b2_delete_file_version makes the implementation easier. It's quite possible to do without it, but saves us a step, and makes things faster, if the name is passed in. We are considering removing that requirement in a future version of the API.

ppolewicz referenced this pull request Dec 31, 2015
Each command will now print helpful info if you
get its arguments wrong, without spewing out info
about all commands.
@ppolewicz
Copy link
Collaborator Author

@bwbeach #20 modifies the same code that I am going to modify. As you intend to accept #20, I'll merge it when working on #11 to avoid a massive conflict.

@ppolewicz
Copy link
Collaborator Author

@bwbeach I am removing print 'Getting upload URL...' as it breaks abstraction in a major way and I do not think it is very important to communicate this to the user. We can add it back later if it is needed after all.

@bwbeach
Copy link
Contributor

bwbeach commented Jan 3, 2016

Sounds good.

On Jan 3, 2016, at 08:38, Paweł Polewicz notifications@github.com wrote:

@bwbeach https://github.com/bwbeach I am removing print 'Getting upload URL...' as it breaks abstraction in a major way and I do not think it is very important to communicate this to the user. We can add it back later if it is needed after all.


Reply to this email directly or view it on GitHub #11 (comment).

@bwbeach
Copy link
Contributor

bwbeach commented Jan 3, 2016

How close are you on #11? If you're close, I could wait on #20.

I'll go ahead an pull in #20 later today if I don't hear from you.

On Jan 2, 2016, at 17:22, Paweł Polewicz notifications@github.com wrote:

@bwbeach https://github.com/bwbeach #20 #20 modifies the same code that I am going to modify. As you intend to accept #20 #20, I'll merge it when working on #11 #11 to avoid a massive conflict.


Reply to this email directly or view it on GitHub #11 (comment).

@ppolewicz
Copy link
Collaborator Author

I am close. I will finish tonight.
Changes in upload_file are are hard. Everything is glued together to everything in there.

It is my fault that #20 will have conflicts and I can see that you intend to merge it in, so in order to avoid putting additional burden on @cederberg, I will try to use git magic to merge it and resolve the conflicts myself. It has to be in the right order with my commits or otherwise the conflict will be massive.

@ppolewicz
Copy link
Collaborator Author

@bwbeach there is code to cope with the fact that the API has returned a dictionary after uploading a file, but the dictionary did not contain the fileId key. I suspect that an early version of b2 api did that. Can I remove it now and assume fileId will be returned properly?

@bwbeach
Copy link
Contributor

bwbeach commented Jan 3, 2016

I think that was an unclear way of checking whether an error was returned, but there are better checks for it now. Feel free to remove it and clean up the code.

On Jan 3, 2016, at 12:33, Paweł Polewicz notifications@github.com wrote:

@bwbeach https://github.com/bwbeach there is code to cope with the fact that the API has returned a dictionary after uploading a file, but the dictionary did not contain the fileId key. I suspect that an early version of b2 api did that. Can I remove it now and assume fileId will be returned properly?


Reply to this email directly or view it on GitHub #11 (comment).

@bwbeach
Copy link
Contributor

bwbeach commented Jan 4, 2016

I'll wait until you're done.

  • BrianB

On Jan 3, 2016, at 12:12, Paweł Polewicz notifications@github.com wrote:

I am close. I will finish tonight.
Changes in upload_file are are hard. Everything is glued together to everything in there.

It is my fault that #20 #20 will have conflicts and I can see that you intend to merge it in, so in order to avoid putting additional burden on @cederberg https://github.com/cederberg, I will try to use git magic to merge it and resolve the conflicts myself. It has to be in the right order with my commits or otherwise the conflict will be massive.


Reply to this email directly or view it on GitHub #11 (comment).

@ppolewicz
Copy link
Collaborator Author

I started work on upload_file, which required refactoring of error handling, which required refactoring of retry mechanic, which required refactoring of OpenUrl, and so on. One thing led to another and according to git diff --stat, 465 LOC were added/deleted. It is 2 AM and my programming skills are getting weaker.

IMO it is not a good idea to merge those new changes with the previous changes. Therefore lets execute plan B: merge it as it currently is and do the remaining items in other pull requests.

I will coordinate the merging and testing of #20 in its pull request.

@ppolewicz
Copy link
Collaborator Author

just a moment, test failure

@ppolewicz
Copy link
Collaborator Author

It was a bad git add, I fixed it. Ready for merge.

@ppolewicz ppolewicz changed the title Library layer Library layer - first part Jan 4, 2016
bwbeach added a commit that referenced this pull request Jan 4, 2016
@bwbeach bwbeach merged commit c16c223 into Backblaze:master Jan 4, 2016
@ppolewicz ppolewicz deleted the library_layer branch January 5, 2016 23:43
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 this pull request may close these issues.

None yet

2 participants