-
Notifications
You must be signed in to change notification settings - Fork 5
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
Get dependencies from original symbol or file lookup #67
Get dependencies from original symbol or file lookup #67
Conversation
* Allow skipping check_method_available * When descriptors are looked up by symbol, use the file descriptors returned in dependency resolution Signed-off-by: Aidan Jensen <aidan@artificial.com>
985a251
to
386a868
Compare
Signed-off-by: Aidan Jensen <aidan@artificial.com>
eea5110
to
40b249b
Compare
@artificial-aidan - just wanted to let you know that I'll give this a thorough review tomorrow night. Thanks for all this work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to admit - to some degree, the use case is a bit over my head - but I think I'm picking up enough to offer some starting feedback.
As always, thanks so much for the contribution!
Signed-off-by: Aidan Jensen <aidan@artificial.com>
Signed-off-by: Aidan Jensen <aidan@artificial.com>
Signed-off-by: Aidan Jensen <aidan@artificial.com>
@ViridianForge Cleaned some stuff up, addressed comments, and added a sync implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
Looks good! If there's anything else you need to do to unblock your workflows, throw up an issue so we can track it, otherwise I'll start working on getting the next release out soon.
That's all for now! Thanks for reviewing it. |
Background here: grpc/grpc#32899
The logic isn't my best work, so definitely open to input on that. The gist of it is to use the file descriptor protos that are returned in the dependency resolution, but the spec didn't specify an order, so I had to do extra logic around lookup. I may go look at how other languages to it, to see if they unofficially assume the order of dependencies will be correct.
This is just the async side, if the feedback is good I'm happy to port it to the sync side as well.
Another change I made was for the test files. I used an empty descriptor pool for the client creation, I found that the descriptors were already loaded, and the test's weren't actually exercising the lookup functionality. I believe this is because the test process loads the server, which loads the protos into its pool. By using an empty pool for the client, we force the exercising of the lookup mechanisms.
Additionally, an optional
skip_check_method_available
was added to the clients. This follows the above in trying to optimize the speed at which we can reflect and call a method.