Using futures in batched requests

[dhermes]

Has #810 as diffbase.

@tseaver Note this does not support the Iterator subclasses, but I have tested with a fair amount of other requests.

It does enable GET requests, but does so in a somewhat incomplete way.

The main issues are

1. In Iterator.get_next_page_response

self.next_page_token = response.get('nextPageToken')

fails when response is a _Future
2. In _BucketIterator.get_items_from_response

for item in response.get('items', []):

fails when response is a _Future

The first is easy to deal with (e.g. hasattr(response, 'get')) but the second is not as easy because we need to start an and iterator before knowing what will end up in the response.

[coveralls]

Changes Unknown when pulling b50d446 on dhermes:add-future-type into * on GoogleCloudPlatform:master*.

[coveralls]

Coverage remained the same at 100.0% when pulling 527cfd1 on dhermes:add-future-type into f08e71b on GoogleCloudPlatform:master.

[coveralls]

Coverage remained the same at 100.0% when pulling f2a9c7d on dhermes:add-future-type into 028a15d on GoogleCloudPlatform:master.

[dhermes]

@tseaver As with #810 I have replaced the original contents from this and placed them in https://github.com/dhermes/gcloud-python/tree/add-future-type-OLD

I was still not able to avoid having each of _helpers and batch reach into each other. From batch:

    def set_future_value(self, value):
        ...
        if self.owner is not None:
            self.owner._properties = value

and from _helpers

    def _set_properties(self, value):
        ...
        if hasattr(value, 'owner'):
            value.owner = self

[coveralls]

Changes Unknown when pulling ce9e260 on dhermes:add-future-type into * on GoogleCloudPlatform:master*.

[dhermes]

@tseaver Bump

[coveralls]

Coverage remained the same at 100.0% when pulling b1e4f7d on dhermes:add-future-type into 286207b on GoogleCloudPlatform:master.

[coveralls]

Coverage remained the same at 100.0% when pulling f48620f on dhermes:add-future-type into 286207b on GoogleCloudPlatform:master.

[dhermes]

@tseaver PTAL. I added two new commits to move towards what we discussed earlier. I'll likely finalize the 4 commits into 1 or 2 once we've finished review, but wanted to keep the "history" of ideas around.

Note that still "unsupported" are:

• _BucketIterator
• _BlobIterator
• (Blob|Bucket).exists() (FYI these should be consolidated into _PropertyMixin someday when STORAGE: Does a connection need to be bound to anything other than a Batch or Iterator? #728 is done)
• Anything in ACL, which we still need to discuss (tried to in Storage "Interface Driven Design" Document #761)

[tseaver]

Ugh, sorry -- those comments are on the individual commits.

[coveralls]

Coverage remained the same at 100.0% when pulling 4a97272 on dhermes:add-future-type into 286207b on GoogleCloudPlatform:master.

[tseaver]

LGTM

[dhermes]

@tseaver We still have some pending discussion points.

Things to resolve:

• Never addressed issue of broken / different behavior of
  (Blob|Bucket).exists().
• Use of the dummy argument
• Should we to support or disable paging in a batch? I am leaning towards disabling, but currently we have no way to send the storage.<NOUN>.list requests without using the paging utils.
• We currently have "inconsistent" support for GET requests. How to move forward?

Also, are you OK with me squashing into fewer ad-hoc commits here or should I submit a new PR?

[dhermes]

@tseaver PTAL

[tseaver]

Never addressed issue of broken / different behavior of (Blob|Bucket).exists().

• Use of the dummy argument

The superclass defines the contract for overridable methods: it must use the same argument names as the ones in the subclasses. This would be another case for an exception to the line-markers (assuming we don't want to disable the check altogether, which is reasonable in my mind for arguments, as opposed to locals defined in the suite).

• Should we to support or disable paging in a batch? I am leaning towards disabling, but currently we have no way to send the storage..list requests without using the paging utils.

I remain pretty unconvinced of the utility of any GET methods in a batch, so I'm fine with disabling the "collection GET" (or crippling them) inside a batch.

We currently have "inconsistent" support for GET requests. How to move forward?

I'm not sure we have a good story at all for how applications would use GET requests inside a batch.

Also, are you OK with me squashing into fewer ad-hoc commits here or should I submit a new PR?

I'm not fussy about the number of commits in a PR: I certainly wouldn't submit a new one just to clean up history. If squashing them makes you feel better about mergeing, I'm OK with it (but it does mean an indefinitely long delay for the merge, given the current Travis delays).

[coveralls]

Coverage remained the same at 100.0% when pulling 301c945 on dhermes:add-future-type into 8b5c5b9 on GoogleCloudPlatform:master.

[coveralls]

Changes Unknown when pulling 4564992 on dhermes:add-future-type into * on GoogleCloudPlatform:master*.

[dhermes]

@tseaver I just merged, we can continue the discussion when dealing with crippling the iterators.

You never mentioned if the behavior of Blob.exists() (and Bucket.exists()) was concerning.

[tseaver]

Hmm, why doesn't the comment in Bucket.exists apply to Blob.exists, too? Surely we don't want to wipe out blob metadata due to an existence check?

[dhermes]

Oversight. My bad.

[dhermes]

Gonna wrap it into the PR for turning off batching for iterators.