-
Notifications
You must be signed in to change notification settings - Fork 137
Add support for gRPC binary metadata values #1791
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
fa18a14 to
2235a4f
Compare
| let Some(headers) = headers else { | ||
| return (None, None); | ||
| }; | ||
| let mut ascii_headers = HashMap::default(); |
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.
Possible optimization is to create the hashmap with the same capacity as the initial headers map. Only downside I could see would be if the headers were mostly binary.
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.
This is once per native client + on clientUpdateHeader. Neither of them should be called frequently enough to justify efforts on performance optimization IMO.
But if we'd indeed want to go there, I'd initiate both hashmaps at the capacity of the initial headers map, up to a certain upper limit (something like 128 or 256). That should optimize performance in all cases while providing a reasonable upper limit on how much memory we may allocate uselessly.
But honestly, I wouldn't mind that one.
1b17bc4 to
def0d6c
Compare
def0d6c to
b9ec854
Compare
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.
Awesome, thanks!
What was changed
Added ability to send binary gRPC headers via
NativeConnection.Also added a test to verify that binary headers were accepted by
Connection.Why?
Make progress on temporalio/features#671
Checklist
Closes [Feature Request] Ensure gRPC binary metadata headers are supported #1778
How was this tested:
Added setting and sending of binary headers at both the client and call level for both native and the standard client connection.
Any docs updates needed?
I believe the type expansion from
stringtostring | Bufferis the only user facing change and that change should appear in the API reference.Each commit is reviewable on its own.