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

Implement functionality to terminate ndt7 tests early given a client parameter #390

Merged
merged 4 commits into from
Jun 27, 2023

Conversation

cristinaleonr
Copy link
Contributor

@cristinaleonr cristinaleonr commented Jun 26, 2023

This PR implements functionality to terminate ndt7 download tests early when the client passes in an early_exit parameter in the request.

The changes were tested by running the server locally and making a request from ndt7-client-go.

$ go run main.go -service-url ws://localhost/ndt/v7/download?early_exit=250
download in progress with localhost
Avg. speed  : 16630.5 Mbit/s
download: complete
         Server: localhost
         Client: 172.17.0.1
        Latency:     0.0 ms
       Download: 16630.5
         Upload:     0.0
 Retransmission:    0.12 %

$ go run main.go -service-url ws://localhost/ndt/v7/download?early_exit=200
download failed: websocket: bad handshake

download: complete
         Server: localhost
         Client:
        Latency:     0.0
       Download:     0.0
         Upload:     0.0
 Retransmission:    0.00
exit status 1

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.

Reviewed 2 of 7 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @cristinaleonr)


ndt7/download/sender/sender.go line 21 at r1 (raw file):

// EarlyExitParams defines the parameters for the sender to end the
// test early.
type EarlyExitParams struct {

Please name this only spec.Params.


ndt7/download/sender/sender.go line 101 at r1 (raw file):

			}
			// End the test once enough bytes have been acked.
			if params.IsEarlyExit && m.TCPInfo.BytesAcked >= params.MaxBytes {

I recommend additionally checking that TCPInfo is not nil.


ndt7/handler/handler.go line 261 at r1 (raw file):

		}

		value := values[0]

What happens if early_exit= is given?

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 (waiting on @stephen-soltesz)


ndt7/download/sender/sender.go line 21 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Please name this only spec.Params.

Done.


ndt7/download/sender/sender.go line 101 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I recommend additionally checking that TCPInfo is not nil.

Done.


ndt7/handler/handler.go line 261 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

What happens if early_exit= is given?

It takes the parameter value as "", the condition below becomes true, and it returns an error. I added a test case for this.

This is also the output from the client:

$ go run main.go -service-url ws://localhost/ndt/v7/download?early_exit=
download failed: websocket: bad handshake

download: complete
         Server: localhost
         Client:
        Latency:     0.0
       Download:     0.0
         Upload:     0.0
 Retransmission:    0.00
exit status 1

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.

I'm sorry - I put a typo in my comment about sender.Params.

With that change :lgtm:

Reviewed 2 of 5 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @cristinaleonr)


ndt7/download/sender/sender.go line 21 at r1 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Done.

I'm so sorry - I wrote spec but meant to leave it in the sender package. I just wanted the shorter type name. /shamecube me

Would you please return the type definition to this package with the short name?

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: :shipit: complete! 1 of 1 approvals obtained (waiting on @stephen-soltesz)


ndt7/download/sender/sender.go line 21 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

I'm so sorry - I wrote spec but meant to leave it in the sender package. I just wanted the shorter type name. /shamecube me

Would you please return the type definition to this package with the short name?

Yes, done.

@cristinaleonr cristinaleonr merged commit 5824ad0 into main Jun 27, 2023
@cristinaleonr cristinaleonr deleted the sandbox-cristinaleon-early-exit branch June 27, 2023 00:28
@cristinaleonr cristinaleonr mentioned this pull request Sep 14, 2023
7 tasks
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