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

add: naive implementation of a minimum-throughput body with tests #1627

Closed
wants to merge 1 commit into from

Conversation

Velfi
Copy link
Contributor

@Velfi Velfi commented Aug 10, 2022

Motivation and Context

#1562

Description

Exactly what it says on the tin

Testing

tests are included

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

Great work! This is a good first step! I don't think there's a ton of work to make this mergeable. The tricky part will be to figure out how this should be configured, and what good defaults are.

// Will send slightly less that 1 byte per second because ASCII digits have a size
// of 1 byte and we sleep for 1 second after every digit we send.
tx.send(Ok(format!("{}", i))).await.expect("failed to send");
async_sleep.sleep(Duration::from_secs(1)).await;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally, we should be able to fake time keeping in the MinimumThroughputBody so that we don't need to sleep in tests. Essentially, be able to replace the source of Instant::now() inside of MinimumThroughputBody.

Comment on lines +62 to +63
let actual_bytes_per_second =
total_bytes_read as f64 / time_elapsed_since_first_poll.as_secs_f64();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think to be accurate for longer downloads, and to avoid consuming excess memory, you'll need to have a sliding window over the throughput_logs. It's probably as simple as popping the front of the vec until the first entry is no longer older than the configured duration (before doing this calculation).

Additionally, we probably shouldn't do this comparison at all unless the configured duration has elapsed already. Otherwise, there's a chance that a slow-to-start download would always result in failure since it would only have one data point.

}
}

#[tokio::test]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm feeling mathy today, bear with me...

I think a good test to add may be a shrinking sine wave. Something like (abs(sin(x))+0.5) * max(0, (-0.05x+1)), which may require a little bit of tweaking.

With that function, it should start off happy despite some significant drop offs in speed periodically, but eventually (after 20 seconds or so) get to a point where it needs to timeout.

Another good test is a straight line going up starting at 1, something like x+1, just to make sure it doesn't break on slow starts.

And then, another good one would be a full minute of good throughput, then 0.5 seconds of zero throughput, followed by good throughput again.

@jdisanti
Copy link
Collaborator

Closing this until we pick up #1562 again.

@jdisanti jdisanti closed this Dec 20, 2022
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.

2 participants