-
Notifications
You must be signed in to change notification settings - Fork 126
RSDK-12391: Optimize modular resource communication. #5403
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
base: main
Are you sure you want to change the base?
Conversation
module/module.go
Outdated
|
|
||
| c, err := m.getLocalResource(ctx, name) | ||
| if err == nil { | ||
| deps[name] = c |
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.
just thought about this, but if the dependency in question always rebuild and the dependency is reconfigured, this would cause issues since the dependent resource will not get a signal to reconfigure with the rebuilt resource
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.
@cheukt added the test you suggested and it is indeed currently failing. Thanks for catching this!
Can you take a look and recommend a path forward? IIUC, only the RDK knows about the actual dependencies. So I think I'm constrained to having the ReconfigureRequest resulting in some special response?
Or maybe we can try to keep it internal to the module because client objects to other modules work fine? And we can have modules keep track which resources they've created "special" dependency objects to?
Or try to do that thin wrapper where a resource can be swapped out under the hood (effectively how clients work)? At the cost of writing wrapper code?
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.
having ReconfigureRequest return something different might be too heavy handed and unnecessary for anything outside of the module. there'll be some compatibility issues too, since the viam-server will have to learn to handle that response correctly.
either having module keep track of resources with special dep relationships / thin wrapper can both work. Thin wrapper will emulate the current relationship better, though I'm not sure if it's doable without a superclient/lots of boilerplate.
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.
gonna mark as needs changes - would approve if just the server.go file changes
Probably doesn't mean much, but the second commit brings this "get point cloud" from "grpc size limit reached" to success in microseconds. A working point cloud of ~10MB goes from 500ms -> 300ms with the RDK middleman optimization. And also microseconds with the inter-module optimization.