Closed artificial-aidan closed 7 months ago
@artificial-aidan - just wanted to let you know that I'll give this a thorough review tomorrow night. Thanks for all this work!
@ViridianForge Cleaned some stuff up, addressed comments, and added a sync implementation
That's all for now! Thanks for reviewing it.
Background here: https://github.com/grpc/grpc/issues/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.