This repository was archived by the owner on Sep 30, 2024. It is now read-only.
executors: multi-queue: enable client to submit heartbeat for multiple queues#52525
Merged
sanderginn merged 41 commits intoMay 31, 2023
Conversation
Collaborator
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits 41275d7 and 42be886 or learn more. Open explanation
|
Contributor
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 80efc59...c36c55e.
|
Contributor
Piszmog
reviewed
May 30, 2023
Piszmog
left a comment
Contributor
There was a problem hiding this comment.
Looks good, just a couple items
Piszmog
reviewed
May 31, 2023
| _, body, err := c.client.Do(ctx, req) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| _, body, doErr := c.client.Do(ctx, req) |
Contributor
There was a problem hiding this comment.
What is the reason for having all the errors be uniquely named?
Contributor
Author
There was a problem hiding this comment.
I noticed the observability routine using the named return variable pointer, so it seems to me that reusing that variable would make the observation kind of pointless?
Of course that would only ask for one other error. Should the observation receive its own error?
Contributor
There was a problem hiding this comment.
Yea, the named returns and observability are weird.. I had to play around with an example to understand better. Take the below example
func Foo(x, y bool) (s string, err error) {
defer func() {
fmt.Println(s, err)
}()
if x {
err = fmt.Errorf("failed")
return "", err
}
if y {
return "foo", nil
}
return "bar", nil
}When the defer function runs, the s and err will be set to the values in the return statement. So even though I set err to a value (overriding the named err), the return statement still sets err to the same thing.
If I update the code to,
if x {
err = fmt.Errorf("failed")
return "", fmt.Errorf("failed again")
}err in the defer will be "failed again" even though I am setting it explicitly.
So by naming it something else and returning the new named error, will still set the observability error to it.
Contributor
Author
There was a problem hiding this comment.
Thanks for that explanation, makes it more clear 👍
I'll change back to a single err in the parent PR then
4 tasks
sanderginn
referenced
this pull request
Jun 4, 2023
…2016) - Closes https://github.com/sourcegraph/sourcegraph/issues/50614 Depends on the following issues: - https://github.com/sourcegraph/sourcegraph/issues/50616 - Merged in https://github.com/sourcegraph/sourcegraph/pull/52020 - https://github.com/sourcegraph/sourcegraph/issues/51656 - Merged in https://github.com/sourcegraph/sourcegraph/pull/52381 - https://github.com/sourcegraph/sourcegraph/issues/51658 - Merged in https://github.com/sourcegraph/sourcegraph/pull/52525 Due to the dependency on the work listed above, all PRs were ultimately merged into this branch, so the client side implementation got a little bit messy to review, for which I apologize. The initial intent of this PR, closing #50614, was reviewed before the merges occurred. ## Demos ### Scenario 1: old Sourcegraph version, new executor version #### batches, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/6d325326-d4f5-4e6f-a5e0-0918dbde07bf #### codeintel, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/ff3a87db-ef07-4221-a159-73b93a6ae55b ### Scenario 2: new Sourcegraph version, old executor version #### batches, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/c66a9b9f-b745-4abc-bf87-9077dd5f2959 #### codeintel, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/785d7f34-630f-4c2a-abb6-82ca3b896813 ### Scenario 3: new Sourcegraph version, new executor version #### batches, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/69c5c06b-ea1c-47f7-9196-e3f108794493 #### codeintel, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/4e791d98-06cd-4fa7-93dd-81be0a73c744 #### batches + codeintel in parallel, multi queue https://github.com/sourcegraph/sourcegraph/assets/2979513/004608dd-f98c-4305-aeb3-a01444c12224 ## Initial PR description (enable client to dequeue from multiple queues) This PR updates the client's `Dequeue` method to dequeue from multiple queues. It's backwards compatible with single-queue configurations. The field `Job.Queue` is only set when dequeuing from the general `/dequeue` endpoint, so the job `MarkXxx` methods default to single-queue behaviour if the job does not specify a queue name. Unrelated to the linked issue, the multi-handler will log and return no content early in the event of an empty queue list in a dequeue request (although this should never occur). ## Test plan - [x] Unit tests - [x] Local testing - [x] Dogfood testing - [x] Demo <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles -->
ErikaRS
referenced
this pull request
Jun 22, 2023
…2016) - Closes https://github.com/sourcegraph/sourcegraph/issues/50614 Depends on the following issues: - https://github.com/sourcegraph/sourcegraph/issues/50616 - Merged in https://github.com/sourcegraph/sourcegraph/pull/52020 - https://github.com/sourcegraph/sourcegraph/issues/51656 - Merged in https://github.com/sourcegraph/sourcegraph/pull/52381 - https://github.com/sourcegraph/sourcegraph/issues/51658 - Merged in https://github.com/sourcegraph/sourcegraph/pull/52525 Due to the dependency on the work listed above, all PRs were ultimately merged into this branch, so the client side implementation got a little bit messy to review, for which I apologize. The initial intent of this PR, closing #50614, was reviewed before the merges occurred. ## Demos ### Scenario 1: old Sourcegraph version, new executor version #### batches, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/6d325326-d4f5-4e6f-a5e0-0918dbde07bf #### codeintel, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/ff3a87db-ef07-4221-a159-73b93a6ae55b ### Scenario 2: new Sourcegraph version, old executor version #### batches, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/c66a9b9f-b745-4abc-bf87-9077dd5f2959 #### codeintel, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/785d7f34-630f-4c2a-abb6-82ca3b896813 ### Scenario 3: new Sourcegraph version, new executor version #### batches, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/69c5c06b-ea1c-47f7-9196-e3f108794493 #### codeintel, single queue https://github.com/sourcegraph/sourcegraph/assets/2979513/4e791d98-06cd-4fa7-93dd-81be0a73c744 #### batches + codeintel in parallel, multi queue https://github.com/sourcegraph/sourcegraph/assets/2979513/004608dd-f98c-4305-aeb3-a01444c12224 ## Initial PR description (enable client to dequeue from multiple queues) This PR updates the client's `Dequeue` method to dequeue from multiple queues. It's backwards compatible with single-queue configurations. The field `Job.Queue` is only set when dequeuing from the general `/dequeue` endpoint, so the job `MarkXxx` methods default to single-queue behaviour if the job does not specify a queue name. Unrelated to the linked issue, the multi-handler will log and return no content early in the event of an empty queue list in a dequeue request (although this should never occur). ## Test plan - [x] Unit tests - [x] Local testing - [x] Dogfood testing - [x] Demo <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hierarchy:
Test plan