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

Remove id from Session message #784

Merged
merged 7 commits into from
Nov 14, 2022
Merged

Remove id from Session message #784

merged 7 commits into from
Nov 14, 2022

Conversation

ccifra
Copy link
Member

@ccifra ccifra commented Nov 8, 2022

What does this Pull Request accomplish?

  • Having both name and id is somewhat confusing
  • It makes using the session in interactive tools like bloom RPC difficult, since oneof is a little complex to use.

I don't think ID should ever be used. There are a lot of advantages to keeping the data on the wire "time invariant" or independent on server state. For example, we are trying to do batching to improve throughput when doing "cloud-based measurements" as part of various LTIs and hopefully products.

Even if we remove id we can still maintain the current behavior by generating a name if the client does not specify. Clients who use name should not need to make any changes to their code. The wire format does not break for clients who use name, so we don't even need to require clients change. The wire format does not really break for clients that use id since they can't set it and our server will stop setting it. Clients who use id should be treating Session as an opaque structure since there is nothing they can do with name.

Why should this Pull Request be merged?

General improvement to grpc-device.

What testing has been done?

Existing test suite updated.

Motivation:
* Having both name and id is somewhat confusing
* It makes using the session in interactive tools like bloom RPC difficult, since oneof is a little complex to use.

I don't think ID should ever be used.  There are a lot of advantages to keeping the data on the wire "time invariant" or independent on server state.  For example we are trying to do batching to improve throughput when doing "cloud based measurements" as part of various LTIs and hopefully products.

Even if we remove id we can still maintain the current behavior by generating a name if the client does not specify.
Clients who use name should not need to make any changes to their code.
The wire format does not break for clients who use name so we don't even need to require clients change.
The wire format does not really break for clients that use id since they can't set it and our server will stop setting it.
Clients who use id should be treating Session as an opaque structure since there is nothing they can do with name.
@astarche astarche added the binary-breaking Change to proto file that requires client updates label Nov 9, 2022
@ccifra
Copy link
Member Author

ccifra commented Nov 9, 2022

Ready for review.

@astarche
Copy link
Collaborator

It looks (to me) like all of the examples are OK. Either way, there should be a release task to run all the examples, especially with some major changes this release. This is something that mypy should catch, so another case for expanding which drivers pass mypy so we can validate_examples on them on the CI.

Nothing needed for this PR. @reckenro can you see if there's a good place to track that release task?

@reckenro
Copy link
Collaborator

It looks (to me) like all of the examples are OK. Either way, there should be a release task to run all the examples, especially with some major changes this release. This is something that mypy should catch, so another case for expanding which drivers pass mypy so we can validate_examples on them on the CI.

Nothing needed for this PR. @reckenro can you see if there's a good place to track that release task?

Yeah, I'm starting an email thread with our PO (Harsha) for where to track this among other tasks with the next release. I'm guessing it'll become a sustaining item in our backlog but whatever the case I'll capture this along with that Release related work.

@ccifra ccifra merged commit ca6a68a into main Nov 14, 2022
@ccifra ccifra deleted the chrisc-RemoveSessionId branch November 14, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binary-breaking Change to proto file that requires client updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants