FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Record decision about optional fields as ADR. by coryan · Pull Request #1358 · googleapis/google-cloud-cpp · GitHub

Record decision about optional fields as ADR. - #1358

Merged
coryan merged 2 commits into
googleapis:masterfrom
coryan:storage-metadata-fields-and-optional
Oct 31, 2018
Merged

Record decision about optional fields as ADR.#1358
coryan merged 2 commits into
googleapis:masterfrom
coryan:storage-metadata-fields-and-optional

Conversation

coryan commented Oct 30, 2018
edited
Loading

Copy link
Copy Markdown
Contributor

This is just rewriting #934 as a ADR [skip ci].


This change is 

This is just rewriting googleapis#934 as a ADR [skip ci].
coryan added api: storage Issues related to the Cloud Storage API. type: process A process-related concern. May include testing, release, or the like. labels Oct 30, 2018
coryan requested review from frankyn and houglum October 30, 2018 03:12
googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 30, 2018

frankyn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

houglum left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

frankyn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

But wait, after chatting with @houglum my scenario wasn't called out during his discussion.

Scenario:
User requests metadata with fields=name for a Bucket resource. The resource has versioning enabled but the Bucket metadata will be set to false, confusing the new user that versioning is disabled.

This is an accidental use-case that a user can make them to shoot themselves in the foot.

We should discuss this more, I misunderstood the state of the conversation.

coryan commented Oct 31, 2018

Copy link
Copy Markdown
Contributor Author

Scenario:
User requests metadata with fields=name for a Bucket resource. The resource has versioning enabled but the Bucket metadata will be set to false, confusing the new user that versioning is disabled.

So this hypothetical user is sophisticated enough to know that they can ask the server to not returns some fields, yet they are confused when one of the values they explicitly filtered out does not match the value in the server?

I do not see it. I think folks sophisticated enough to request a subset of the fields know that all other fields cannot possibly have meaningful values.

And we should be optimizing for he common case, which I believe is no filtering, in that case have to treat each field as potentially optional is super awkward, while the filtering case is harder to deal with, but you are doing something special. In other words, we should be making normal things easy, and weird things possible.

PS: Not that it matters, but versioning happens to be optional, but I believe you were making a broader point about any field that just gets a weird default when the server does not send it back.

frankyn commented Oct 31, 2018
edited
Loading

Copy link
Copy Markdown
Contributor

Apologies, my last comment as you mentioned was a generalized statement.

Preface: I don't have any external developer feedback here, and it is more a thought.
Let me change the perspective on my last comment to a more relatable perspective (at least for me).

An existing code base that uses the GCS C++ client library was first implemented by an experienced developer with field selection and some time after a new developer on the team hits this issue when accessing an unselected field. The developer may incur hours of debugging the issue because the value isn't consistent with the GCS service. The developer learns a lot about GCS the hard way.

The alternative to this scenario is the new developer attempts to use a value not selected and finds
the value is not set with optional<T>::has_value(). "Why isn't it set?" After reviewing the code, the developer finds that the code has selected specific fields and reviews API documentation with context on how to use field selection.

coryan commented Oct 31, 2018

Copy link
Copy Markdown
Contributor Author

That is a better argument, thanks.

To make life easier in your scenario though, every developer has to use the awkward optional field API. And then have to do this always, even if they never use field filtering. They are paying for what they don't use. And I think we both agree that most of the time folks are not going to use field filtering.

I do not like this tradeoff, making most folks pay, most of the time, for something that happens rarely is just not ideal.

And look to the other APIs: proto3 no longer has has_field() for primitive types and strings. That would make the GCS API inconsistent with all the other C++ APIs based on protos, where there is always a value (false or 0 but a value).

frankyn commented Oct 31, 2018

Copy link
Copy Markdown
Contributor

Agreed, we are on the same page on the given overhead for use-cases that may not be encountered often that would impact all developers using the library. I don't like it either, but I'd like to be make sure we address this appropriately.

With respect to proto3, GCS API is proto2 API maybe there's rationale to have optional in this case? It's not consistent with other libraries, but that's because it isn't a proto3 API.

frankyn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

One request and LGTM.

coryan commented Oct 31, 2018

Copy link
Copy Markdown
Contributor Author

PTAL.

frankyn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, LGTM

coryan merged commit cd65fcb into googleapis:master Oct 31, 2018
coryan deleted the storage-metadata-fields-and-optional branch October 31, 2018 20:59

kman242424 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement. type: process A process-related concern. May include testing, release, or the like.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL