-
Notifications
You must be signed in to change notification settings - Fork 1
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
Set the global start time when the first connection is established #28
Conversation
Pull Request Test Coverage Report for Build 6706749025
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to test these changes?
Reviewable status: 0 of 1 approvals obtained (waiting on @robertodauria)
pkg/client/client.go
line 90 at r1 (raw file):
recvByteCountersMutex sync.Mutex globalStartTime time.Time
Calling a struct field "global" is a misnomer. Is this just startTime
for this client instance?
There was a problem hiding this 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 @stephen-soltesz)
pkg/client/client.go
line 90 at r1 (raw file):
Previously, stephen-soltesz (Stephen Soltesz) wrote…
Calling a struct field "global" is a misnomer. Is this just
startTime
for this client instance?
It's the common start time that all the streams started by this client share for the purpose of computing the aggregate goodput. sharedStartTime
or just startTime
?
There was a problem hiding this 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 @robertodauria)
pkg/client/client.go
line 90 at r1 (raw file):
Previously, robertodauria (Roberto D'Auria) wrote…
It's the common start time that all the streams started by this client share for the purpose of computing the aggregate goodput.
sharedStartTime
or juststartTime
?
👍
sharedStartTime
works for me.
The test timeout now starts when the first connection is established.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored the code a bit to make testing easier in the future, but did not have time to add tests in this PR. Will add them when I'm back in January, if that works for you.
Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)
pkg/client/client.go
line 90 at r1 (raw file):
Previously, stephen-soltesz (Stephen Soltesz) wrote…
👍
sharedStartTime
works for me.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 1 approvals obtained
This makes sure that we are only counting the elapsed time after at least one stream has started.
This change is