Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cleanup KV HttpClient construction #152

Merged
merged 3 commits into from
Nov 3, 2022
Merged

Cleanup KV HttpClient construction #152

merged 3 commits into from
Nov 3, 2022

Conversation

vlovich
Copy link
Contributor

@vlovich vlovich commented Nov 3, 2022 •
edited
Loading

Remove a bunch of boilerplate used to construct the HttpClient for a KvNamespace.

@vlovich vlovich requested a review from bretthoerner November 3, 2022 20:43
@vlovich vlovich force-pushed the vlovich/make-kv-protected branch from 317ae78 to 7219b87 Compare November 3, 2022 21:27
@vlovich vlovich changed the title Make KV private variables protected Cleanup KV HttpClient construction Nov 3, 2022
bretthoerner
bretthoerner reviewed Nov 3, 2022
View reviewed changes
src/workerd/api/kv.h Outdated Show resolved Hide resolved
bretthoerner
bretthoerner approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@bretthoerner bretthoerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I'd drop the extra allocation just because. :)

@vlovich
Expose getHttpClient convenience method
3384693 
This lets us centralize the boilerplate of constructing the HTTP client.
@vlovich
Cleanup duplicate boilerplate code to use KvNamespace::getHttpClient
f725159 
We had a bunch of copy-pasted code to initialize the HttpClient to the
KV worker. Clean it up to use the new `getHttpClient` helper.
@vlovich
Fix duplicate heap allocations
3ea28f0 
We had been creating a copy of the URL to add to the headers. However,
the URL is guaranteed to live long enough until `client->request` which
is the same lifetime as the headers. Thus the copying of the URL is
unnecessary and can be removed.
@vlovich vlovich force-pushed the vlovich/make-kv-protected branch from a0f5ef9 to 3ea28f0 Compare November 3, 2022 21:59
@vlovich vlovich merged commit c6a5a99 into main Nov 3, 2022
@vlovich vlovich deleted the vlovich/make-kv-protected branch November 3, 2022 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bretthoerner bretthoerner bretthoerner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vlovich @bretthoerner