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.

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

x/build/gerrit: support Contexts? #18742

Closed
kevinburke opened this issue Jan 22, 2017 · 4 comments
Closed

x/build/gerrit: support Contexts? #18742

kevinburke opened this issue Jan 22, 2017 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unreleased

Comments

@kevinburke
Copy link
Contributor

If you are making concurrent requests with the Gerrit API client in x/build/gerrit, it would be nice to be able to coordinate cancelation if the request is no longer necessary.

For example, https://go-review.googlesource.com/#/c/35551/ would fetch Github issues and Gerrit CL's in parallel; it would be nice to cancel one if the other fails.

There's some preliminary discussion about migrating a client library here, on the go-github project: google/go-github#526

Some options, none of which I am very happy about:

  • Change the API.
  • Adding XContext(ctx, args...) methods on the existing Client for every X method, e.g. AbandonChangeContext, CreateProjectContext, etc.
  • Add a ContextClient that has a Context property on the Client struct, and give up on reusing that client across requests.
  • Add a ContextClient that has the same methods, but with a Context parameter as the first argument, so you'd choose which client you want based on whether you want contexts or not.
  • Copy to x/build/gerrit2, x/build/gerritctx or similar with a renamed set of API's.
  • Do nothing.
@dmitshur
Copy link
Contributor

Copy to x/build/gerrit2, x/build/gerritctx or similar with a renamed set of API's.

In theory, with some amount of work, such a package can be automatically generated by source code rewriting of the original package. Then it can be easily kept in sync, it'd be just go generate. But I think it's a bad idea to have 2 packages permanently, if there's no long term plan to eventually join them (which would require breaking APIs).

@bradfitz
Copy link
Contributor

The real bug is #18743

Then we can simply add contexts. We're not going to maintain double the API surface. We can barely maintain 1x the API surface.

@bradfitz bradfitz added this to the Unreleased milestone Jan 22, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 22, 2017
@bradfitz
Copy link
Contributor

@kevinburke, feel free to add contexts.

@gopherbot
Copy link

CL https://golang.org/cl/35559 mentions this issue.

@golang golang locked and limited conversation to collaborators Jan 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@bradfitz @kevinburke @dmitshur @gopherbot