-
Notifications
You must be signed in to change notification settings - Fork 56
Set maximum message length #339
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
Conversation
max_message_length = _get_max_c_int_limit() | ||
self._channel = grpc.insecure_channel( | ||
f"{ip}:{port}", | ||
options=[ |
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 couldn't find a way to set this after the channel is created. So will need to document for when the channel instance is passed.
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.
@mkundu1 Is there a performance penalty for setting a high value here?
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.
@h-krishnan I think ideally we should replace GetStaticInfo
with a streaming rpc. gRPC official website doesn't seem to have any best practice guideline regarding the message size (this is the closest thing I could find, probably the default message size is the best practice). Various thridparty blogs (like this) suggests against changing the message limit or making it too large.
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.
@mkundu1 A streaming API will be difficult with a hierarchical structure. We need to figure out a way to flatten the tree. Maybe return root, its immediate children, immediate children of the children in sequence etc.
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.
That's a pretty big number, what is it if you don't explicitly set the values? What's the reason to even change it? Was there an issue someone encountered?
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.
@h-krishnan Could we use the SerializeToString/ParseFromString methods for protobuf messages and then split and stream as strings?
@dnwillia-work This is for the issue reported by Gitesh (in Solver API teams channel) as the latest version of Fluent's solver API is failing due to message size limit. I have here replicated the C++ code for workspace clients added for the same issue.
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.
OK, I must have missed that.
No description provided.