RSDK-8081 Fix max message size limit #264
Conversation
lia-viam
changed the title
src/viam/sdk/rpc/dial.hpp
Outdated
|
|
||
| namespace viam { | ||
| namespace sdk { | ||
|
|
||
| grpc::ChannelArguments default_channel_args(); |
There was a problem hiding this comment.
This probably doesn't need to be in a public header - I'd expect all the callers to be in .cpp scope.
src/viam/sdk/robot/client.cpp
Outdated
| @@ -354,7 +354,7 @@ std::shared_ptr<RobotClient> RobotClient::at_local_socket(const std::string& add | |||
| const std::string addr = "unix://" + address; | |||
| const char* uri = addr.c_str(); | |||
| const std::shared_ptr<grpc::Channel> channel = | |||
| grpc::CreateChannel(uri, grpc::InsecureChannelCredentials()); | |||
| grpc::CreateCustomChannel(uri, grpc::InsecureChannelCredentials(), default_channel_args()); | |||
There was a problem hiding this comment.
You could also introduce a (SDK private) CreateViamGrpcChannel that did this for you.
There was a problem hiding this comment.
I like this idea. Do we have any SDK private modules outside of components/private ? Maybe this could be in rpc/private? Or whatever else you had in mind
There was a problem hiding this comment.
Yeah, I think rpc/private is the right place.
src/viam/sdk/rpc/dial.cpp
Outdated
| @@ -26,6 +26,14 @@ extern "C" char* dial(const char* uri, | |||
| namespace viam { | |||
| namespace sdk { | |||
|
|
|||
| grpc::ChannelArguments default_channel_args() { | |||
There was a problem hiding this comment.
- In RSDK-6025 - match viam-server's max message size #254 there were three fields set. Just checking that setting two is correct here
- Is there a way we can used named constants for both the client side (this code) and the server side (RSDK-6025 - match viam-server's max message size #254)
There was a problem hiding this comment.
Good idea about the named constant, should definitely be a constexpr somewhere. To your other point, yes these are the only max parameters in ChannelArguments: https://grpc.github.io/grpc/cpp/classgrpc_1_1_channel_arguments.html
src/viam/sdk/tests/test_utils.hpp
Outdated
| auto test_server = TestServer(server); | ||
| auto grpc_channel = test_server.grpc_in_process_channel(args); | ||
| auto grpc_channel = test_server.grpc_in_process_channel(sdk::default_channel_args()); |
There was a problem hiding this comment.
- I think there is another call to this in
test_robot.cpp. - Guessing now that this is why this function is exposed.
There was a problem hiding this comment.
Thanks for catching test_robot.cpp. Building off your other point, maybe we could do something like
- SDK constant for default max message size
- SDK private
CreateViamChannel - Test utils
CreateTestServerChannelwhich would be used here and intest_robot.cpp
?
There was a problem hiding this comment.
That sounds like a good plan, yes.
|
Addressed Andrew's suggestions. Some minor stuff to flag for discussion
|
src/viam/sdk/robot/client.cpp
Outdated
| @@ -354,7 +354,7 @@ std::shared_ptr<RobotClient> RobotClient::at_local_socket(const std::string& add | |||
| const std::string addr = "unix://" + address; | |||
| const char* uri = addr.c_str(); | |||
| const std::shared_ptr<grpc::Channel> channel = | |||
| grpc::CreateChannel(uri, grpc::InsecureChannelCredentials()); | |||
| viam::sdk::impl::create_viam_channel(uri, grpc::InsecureChannelCredentials()); | |||
There was a problem hiding this comment.
You probably don't need the full qualification here.
There was a problem hiding this comment.
you're right, I think I can get away with sdk::impl but impl alone was erroring out because RobotClient uses the pimpl idiom with struct impl
Fixes an issue where the gRPC 4mb message size limit was being hit despite #254
Per this issue discussion we now change the channel args as well: tensorflow/serving#1382 (comment)
This needs to be set every time we create a channel, so I introduced a helper which sets the parameters and passes the args. I believe this is the best we can do since grpc Channel is
finaland only constructible by aCreatefunction and you can't set args on an already-created channelcc @stuqdog @acmorrow