Removing HTTP request in _PropertyMixin.properties.

[dhermes]

NOTE 1: Has #683 as diffbase.

NOTE 2: There are many places here where the tests just give up and call _reload_properties(). This should not be. We should be clear on which methods should be just returning local data and which should make an HTTP request (e.g. update to Bucket.get_logging() here). (I also partially point out this type of problem in #683)

Also removing dead code:

• _PropertyMixin.CUSTOM_PROPERTY_ACCESSORS (and subclasses use of this)
• _PropertyMixin._get_property (only consumer of CUSTOM_PROPERTY_ACCESSORS)

[dhermes]

@tseaver Rebased on top of #683 after merge. PTAL.

This addresses the second of my 4 concerns.

[dhermes]

@tseaver I just took out all the XXX discussion points from the commit and so removed the don't merge label.

There were six places (in the old commit) where I was unsure of what was happening and wanted to discuss:

• Calling _reload_properties before bucket.get_cors()
• Calling _reload_properties before bucket.get_lifecycle()
• Needing 3 mocked HTTP responses to call bucket.get_logging() before and after bucket.enable_logging(...) (2 different places)
• Same as above, but for calling bucket.disable_logging()
• Calling _reload_properties before using bucket.versioning_enabled getter

[tseaver]

I don't have a problem with the implementation, but I'm not sure we have addressed what to do with the "don't make me think" usability issue introduced by removing "eager" loads.

[dhermes]

@tseaver Yeah for sure we've got to get a better handle on that.

But if people typically get a bucket via storage.get_bucket(bucket_name) then properties will already be populated.

Those crafting one by hand via Bucket(bucket_name) may fall into the class of more sophisticated users.

[dhermes]

@tseaver After #683 the implementation is kind of in flux and moving towards a new equilibrium.

I am more worried about being consistent in methods like get_logging() that are essentially just getters.

[dhermes]

@tseaver I restarted the Travis build and it kickstarted coveralls into action

[tseaver]

For the case where missing "eager" messages would hurt: should getters raise an exception (with a message indicating "call load()"?)

[dhermes]

What do you mean by eager messages? Also, should this discussion block this PR? (I see this PR as a tiny piece of moving towards whatever monolith change #632 represents.)

Yes, I think we should raise in some situations. Or provide an load_properties=False boolean in the constructor. Or some other such thing.

When we get the same level of implicit (and lazy-loading) support that we have in datastore, we could also make eager vs. declarative a setting that people could turn on.

[tseaver]

I meant to ask whether attempts to use not-yet-fetched properties should fail? E.g., Bucket.get_cors() will return an empty list if _load_properties has not yet been called, even though the server-side might actually have values; should it be an error?

Essentially, this is the same as the "testing" problem you listed above (where you have to call bucket._reload_properties()): the PR deletes the "eager" tests (which called it implicitly), but leaves those paths in limbo.

[dhermes]

@tseaver Yes I think we should just add a dirty boolean (probably needs a better name) and a timestamp of "last retrieved" / "last synced".

But for things like get_cors(), maybe we want them to make the HTTP request (i.e. not behave like a getter, but like an HTTP request).

[tseaver]

We need to decide then if we are going to keep any "non-dirty" state client side. Having to make multiple HTTP requests to fetch different bits of the state is not exactly performant, compared to the "lazy fetch" we have before this PR series.

[dhermes]

We just need a declarative get / load / reload so that users can populate the contents of _properties

[dhermes]

@tseaver I just rebased (sorry I didn't realize #693 made this PR un-mergable).

Shall we put these concerns in #632 as points to address? We certainly need a "lazy" / "auto" story, but it should be closer to the surface of user interaction and should happen at well-defined times (i.e. in constructor given optional argument).

[coveralls]

Coverage remained the same at 100.0% when pulling 706f7ad on dhermes:storage-properties-no-http into 83f92cc on GoogleCloudPlatform:master.

[tseaver]

I guess we can move forward here. Trying to re-wire the whole API from the inside out is problematic: I'm pretty sure we should be defining the "desired" stories someplace, so that we know when we have arrived.

[dhermes]

@tseaver That's what #632 is for. High-level concepts we want to be true to. (It currently says nothing about an "eager" mode.)

As far as "inside-out", at one point I thought about just starting a brand new module and just porting features over, but decided against it.

[tseaver]

I'd rather have "stories" which outlined what the developer experience actually looks like (vs. the "principles" bit in #632). Our current docs are missing this kind of "narrative" focus, which I feel to be a big deficiency: putting that off in another, tech-writer-maintained place (that doesn't exist yet) leaves the developer side unfocused.

[dhermes]

You can open an issue similar to #632 with our desired dev exp for storage in the meantime and then we can align with that.