Skip to content

Conversation

@chrispine
Copy link

This gzips http requests to the backend. It relies on this and partially fixes this.

@LawnGnome
Copy link
Contributor

I approved this, but I do have a concern with the migration path: since there's no way for the Sourcegraph server to indicate its version before a request is made, we may attempt to speak gzip to a pre-3.21 version that isn't expecting it and doesn't know what to do with it.

One option that does occur to me would be to have the Sourcegraph server returns its version in a Server header: we could put a latch in the src-cli client that doesn't compress requests until it has received a response with the appropriate header and minimum version, at which point future requests will be gzip encoded. This still deals with the campaign issue, since the campaign spec has to be uploaded before any changesets can be, but means that we don't necessarily get the benefits on the first request in a src-cli invocation.

(Spoiler: having a version field in the response is something I'll also be proposing in the versioning RFC.)

@eseliger
Copy link
Member

eseliger commented Oct 7, 2020

Your point is still valid because we do evaluate the search scope before and stuff, but I think campaign specs are created at the very end, since they're bundling changeset specs :)

@LawnGnome
Copy link
Contributor

Your point is still valid because we do evaluate the search scope before and stuff, but I think campaign specs are created at the very end, since they're bundling changeset specs :)

Never trust my memory on a one coffee day. 😆

@mrnugget
Copy link
Contributor

mrnugget commented Oct 8, 2020

I approved this, but I do have a concern with the migration path: since there's no way for the Sourcegraph server to indicate its version before a request is made, we may attempt to speak gzip to a pre-3.21 version that isn't expecting it and doesn't know what to do with it.

How about we bring back the version checks as the first step here? We had this code before switching to the new workflow:

version, err := getSourcegraphVersion(ctx, client)
if err != nil {
return err
}
supportsBaseRef, err := sourcegraphVersionCheck(version, ">= 3.14.0", "2020-03-11")
if err != nil {
return err
}

(Complete code here is ~40 lines and self-contained.)

It does make an additional request, but I think that's negligible for now. And it gives us a back a way, today, to fence features. Once we return the version in a header we can switch out the "get the version" part and keep the check.

@eseliger
Copy link
Member

eseliger commented Oct 8, 2020

It does make an additional request, but I think that's negligible for now.

That's probably right. Why not

@chrispine chrispine merged commit c20c3f5 into main Oct 12, 2020
@chrispine chrispine deleted the cp/gzip-http-requests branch October 12, 2020 20:06
}

return req, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

That means we try to gzip every request — GraphQL, non-GraphQL — now, right? Doesn't that break things, since we only added gzip support to the GraphQL API?

@mrnugget
Copy link
Contributor

This broke the build, because semver is missing in go.mod and go.sum. And it also introduces a data race: multiple goroutines will now access and try to modify the supportsGzip field.

I'll open a PR that fixes this and also try to only use gzip in the campaigns part of src-cli, so we don't unintentionally break something.

@mrnugget
Copy link
Contributor

I opened a follow-up PR that addresses my comments here #343 so that we can release src-cli before branch cut.

scjohns pushed a commit that referenced this pull request Apr 24, 2023
* gzip http requests

* remove unused alias

* add version checks before gzipping

* swallow errors when checking backend version

* I do not know why my editor keeps adding this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

campaigns: large changesets can result in 413 errors

5 participants