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

Send measurement if the test ended before UpdateInterval #92

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

cristinaleonr
Copy link
Contributor

@cristinaleonr cristinaleonr commented Jun 29, 2023

This PR adds functionality to send the measurement through the channel if one is received before params.UpdateInterval. It used to not be likely that a test would terminate before said interval, but this is now possible with the early-exit changes to the server.


This change is Reviewable

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @cristinaleonr)


internal/download/download.go line 44 at r1 (raw file):

			// If the test finished before `params.UpdateInterval` and at least one message
			// has been received, send the measurement data through the channel before exiting.
			if elapsed <= params.UpdateInterval && total > 0 {

nit: there is a race here based on update interval and elapsed time. I can imagine scenarios where the elapsed time below is less than UpdateInterval but by the time we get here it is greater than.

What if we set a boolean below instead? And, report a measurement here only if that boolean is unset?

Copy link
Contributor Author

@cristinaleonr cristinaleonr left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained


internal/download/download.go line 44 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

nit: there is a race here based on update interval and elapsed time. I can imagine scenarios where the elapsed time below is less than UpdateInterval but by the time we get here it is greater than.

What if we set a boolean below instead? And, report a measurement here only if that boolean is unset?

Done.

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@cristinaleonr cristinaleonr merged commit a662702 into main Jul 5, 2023
@cristinaleonr cristinaleonr deleted the sandbox-cristinaleon-early-exit branch July 5, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants