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

Implement new reflection v1 server and public API #5773

Closed
wants to merge 2 commits into from

Conversation

buzzsurfr
Copy link
Contributor

@buzzsurfr buzzsurfr commented Nov 5, 2022

CHANGES:

  • Re-aliases rpb to rv1alphapb in reflection implementation. This allows for adding support for v1 into the registration in the future.
  • Implement a new type serverReflectionServerV1 which implements the v1 service
  • Implement NewServerV1 and RegisterV1 as duplicates that use the v1 server (was necessary for proper testing).

Works on #5684 but will require a future commit that implements both v1alpha and v1.
Depends on merging #5799 (tests will fail until merge and this will be rebased)

RELEASE NOTES:

  • Re-aliases rpb to rv1alphapb in reflection implementation
  • Implement a new type serverReflectionServerV1 and new functions NewServerV1 and RegisterV1 which use reflection v1

@easwars easwars self-assigned this Nov 8, 2022
@zasweq zasweq requested a review from easwars November 8, 2022 18:46
@easwars easwars added this to the 1.52 Release milestone Nov 8, 2022
@easwars
Copy link
Contributor

easwars commented Nov 8, 2022

We want both the v1alpha and the v1 versions of the reflection service to be exported. We cannot remove support for the v1alpha service without making a public announcement about it and giving adequate time for our users to migrate to the new service. Having this in mind, the changes should do the following:

  • the documentation should reflect the fact that both versions are exported
  • the implementation must import both packages (and import alias them appropriately, something like reflectionv1pb and reflectionv1alphapb), and export both services
  • the regenerate script should also pull in both versions

Another thing to take care of, which is not directly related to the changes that are being made in this PR, is to have separate imports for messages and services. See here for an explanation of why we need to have separate imports.

@easwars
Copy link
Contributor

easwars commented Nov 15, 2022

@buzzsurfr : Is this PR only adding the new files? If that is the case, this PR should not change other documentation and source files. If we agree on doing the actual implementation in a follow up PR, we need to change the PR description and associated release notes entry as well.

Also, looks like there is a vet failure in v1alpha for the same typo you fixed on v1 :-(

@buzzsurfr
Copy link
Contributor Author

buzzsurfr commented Nov 16, 2022

@easwars: I fixed the vet issue but haven't rebuilt the tests yet. I'll do so once I clean up the PR.

My intent was to change the documentation locally in the repo to reflect v1 and v1alpha being available. Let's implement v1 as the standard, or build an abstraction layer (interface I'm guessing) so that we can change the default from v1alpha to v1 at a later date. This way it wouldn't affect current builds but would maintain forward compatibility.

Given that it's my first PR on this org, can you provide additional guidance/suggestions? 😄

/remove label:"Status: Requires Reporter Clarification"

@easwars
Copy link
Contributor

easwars commented Nov 16, 2022

Given that it's my first PR on this org, can you provide additional guidance/suggestions? 😄

The reflection package has two APIs:

  • NewServer which takes ServerOptions and returns a implementation of the reflection service
  • Register which takes an interface implemented by the grpc.Server, and internally calls NewServer() and passes some sane default values for other fields of ServerOptions

The reflection package implementation contains the serverReflectionServer type which implements the ServerReflectionInfo() method.

Changes that need to be made for us to support v1 in addition to v1alpha would be as follows (and each can be a separate PR):

  • Update regenerate.sh to pull in v1 pb.gos
  • Implement a new type serverReflectionServerV1 which implements the v1 service
    • As part of this change, refactor serverReflectionServer to pull out common code and share it with serverReflectionServerV1
    • From what I see, we should try to share code which deals with file descriptors, extensions etc because these are not tied to the reflection service
    • Code that actually deals with the request and response message types will have to be duplicated
  • Change the public API
    • Add a new function NewServerV1 which returns the v1 implementation
    • Modify Register to create both implementations and register them on the provided grpc.Server
    • Update package documentation and any other documentation (and/or examples) to indicate that we support both v1 and v2

Since we support Go versions older than 1.18, the implementation will not be able to use generics and hence there will be some amount of code duplication, and that is fine.

Let me know what you think of this approach.

@buzzsurfr
Copy link
Contributor Author

That makes sense to me. I'll split this into 3 PRs. For the last one, I was also considering an interface where we could create a new server that takes the API version as an argument (default to v1alpha for now). Then we can also flip it at a later date.

@easwars
Copy link
Contributor

easwars commented Nov 16, 2022

Thanks @buzzsurfr. We can probably repurpose this PR for first item in the list. Do let me know when you have updated it and I will take another look.

@buzzsurfr
Copy link
Contributor Author

Created #5799 as the first step to pull in protobufs from the remote repo and regenerate. Working on the next two now, but going to close this one and move to the new PR (for a clean history).

@buzzsurfr buzzsurfr closed this Nov 16, 2022
@buzzsurfr buzzsurfr reopened this Nov 16, 2022
@buzzsurfr buzzsurfr changed the title Add new files for reflection v1 Implement new reflection v1 server and public API Nov 17, 2022
@buzzsurfr
Copy link
Contributor Author

@easwars Rebased this branch and reused the PR for the second and third items. Description has been changed. I'm leaving this on draft as the changes is #5799 need to be merged before this PR.

This also does NOT include adding v1 to the default implementation--but this gets the codebase ready to do so.

@easwars easwars closed this Nov 23, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants