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

coordinate successful and failing fetches #6699

Merged

Conversation

stuartnelson3
Copy link
Contributor

Motivation/summary

make sure that the servers all receive their
requests before confirming that a successful
response from one server cancels the shared
context in the fetch code.

How to test these changes

non-functional change

Related issues

closes #5836

make sure that the servers all receive their
requests before confirming that a successful
response from one server cancels the shared
context in the fetch code.
@stuartnelson3 stuartnelson3 added the backport-8.0 Automated backport with mergify label Nov 23, 2021
@stuartnelson3 stuartnelson3 requested review from estolfo and a team November 23, 2021 15:44
@apmmachine
Copy link
Contributor

apmmachine commented Nov 23, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-11-29T12:50:43.263+0000

  • Duration: 42 min 38 sec

  • Commit: 1664e5f

Test stats 🧪

Test Results
Failed 0
Passed 5586
Skipped 19
Total 5605

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /hey-apm : Run the hey-apm benchmark.

  • /package : Generate and publish the docker images.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Contributor

@estolfo estolfo left a comment

Choose a reason for hiding this comment

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

looks good, thanks for doing this

resc <- response{consumer, err}
}()
// Make sure every server has received a request
wg.Wait()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we remove this wait group? It seems to me that the channels are already making the test wait until we've received all the signals. What is this wait group adding on top of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The waitgroup is making sure that each handler has received a request before we attempt to read from any of the channels. Without that, we could potentially read from errc and successc without the third handler ever being invoked.

Copy link
Contributor

Choose a reason for hiding this comment

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

got it, feel free to merge, thanks for the explanation

@stuartnelson3 stuartnelson3 enabled auto-merge (squash) November 29, 2021 12:50
@stuartnelson3 stuartnelson3 merged commit 039bffa into elastic:master Nov 29, 2021
mergify bot pushed a commit that referenced this pull request Nov 29, 2021
make sure that the servers all receive their
requests before confirming that a successful
response from one server cancels the shared
context in the fetch code.

(cherry picked from commit 039bffa)
@stuartnelson3 stuartnelson3 deleted the coordinate-fleet-server-fetch-test branch November 29, 2021 13:46
stuartnelson3 added a commit that referenced this pull request Nov 29, 2021
make sure that the servers all receive their
requests before confirming that a successful
response from one server cancels the shared
context in the fetch code.

(cherry picked from commit 039bffa)

Co-authored-by: stuart nelson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.0 Automated backport with mergify test-plan-skip v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more detailed test for fetching sourcemap from multiple fleet servers
4 participants