DISCUSSION: abstractions of service calls in storage

[silvolu]

Generally speaking, the library functions seem to abstract when service interactions occur from the user. In other words, some operations are local to the object (e.g acl.grant), others interact with the service (e.g. acl.save), while still others interact only some of the time (e.g. bucket.default_object_acl).
If you're going to follow this model (and I'm not sure you should), I think you need to add preconditions to most service calls to ensure that confusing things don't happen.
A good example of this would be the save and reload functions in the ACL class. When should the user call reload()? I don't think it's clear. What happens when the user calls save() but the target ACL has changed on the service side since we last loaded it?

This abstraction is especially dangerous because it works very well when a single client is involved but creates subtle problems when deployed at scale.

/cc @thobrla

[dhermes]

In addition to a more focused / consistent approach, I think the methods in Connection should have no side effects, just as we do in the datastore subpackage.

Just look at something like Connection.delete_bucket. When called with force=True it makes an arbitrary (potentially large) number of delete calls for individual objects. This abstraction should happen on the Bucket class (if at all), not on the Connection. (has been done)

Some other differences / poorly aligned overlap (as of 86acc4a):

• Connection.make_request in storage vs. Connection._request in datastore (big overlap) (_make_request is now protected)
• Connection.api_request in storage vs. Connection._rpc in datastore (public vs. private) (api_request is used a lot elsewhere, so SHOULD be public)
• Connection.__iter__ loops through all buckets (seems out of place). This is used in get_all_buckets. (has been removed)
• Connection.lookup does what Connection.get_bucket (probably) should do. Either way, they are too similar and one should be removed. (has been removed)
• Connection.new_bucket factory should be scrapped and higher quality Bucket constructor implemented (if needed) (ditto on Bucket.from_dict, a classmethod which could be incorporated into the constructor) (has been done)
• Connection.generate_signed_url does not actually make any HTTP request and only relies on the associated credentials object for the connect. Consider making a function? (has been done)
• Connection.__contains__ feels out of place and is a proxy for "does connection have access to this bucket? / does this bucket exist?" ((has been removed)

In addition, there are 10 uses of Connection.api_request outside of the Connection class:

$ git grep '\.api_request' | egrep -v 'storage/connection' | egrep -v test
gcloud/storage/_helpers.py:        self._properties = self.connection.api_request(
gcloud/storage/_helpers.py:        self._properties = self.connection.api_request(
gcloud/storage/acl.py:        found = self.bucket.connection.api_request(method='GET', path=url_path)
gcloud/storage/acl.py:            result = self.bucket.connection.api_request(
gcloud/storage/acl.py:        found = self.key.connection.api_request(method='GET', path=url_path)
gcloud/storage/acl.py:            result = self.key.connection.api_request(
gcloud/storage/bucket.py:            response = self.connection.api_request(method='GET', path=key.path)
gcloud/storage/bucket.py:        self.connection.api_request(method='DELETE', path=key.path)
gcloud/storage/bucket.py:        self.connection.api_request(method='POST', path=api_path)
gcloud/storage/iterator.py:        response = self.connection.api_request(

These should be consolidated inside Connection itself. (Ditto for the 2 uses of build_api_url in storage.key.)

[dhermes]

@thobrla We are trying to distill your concerns further. /cc @tseaver

RE: "others interact only some of the time": this is done mostly for lazy loading, which is a benefit for users. Do you think this is problematic?

RE: "ACL.save() without an ACL.reload()" I've been trying to unravel service calls and but having a hard time keeping track due to the convenience mapping of metadata keys to native Python properties. We have been discussing a bit, but don't have a clear solution / path forward.

Are you aware of

with bucket.batch:
    bucket.enable_versioning()
    bucket.disable_website()

[thobrla]

It is a benefit for users until an error occurs (even a transient service error) and breaks the illusion of atomicity.

Batching configuration changes into a single patch request is fine.

[dhermes]

Closing based on @thobrla comment.