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

Handle slicer Toxic with zero SizeVariation #359

Merged
merged 8 commits into from
Jan 26, 2022

Conversation

areveny
Copy link
Contributor

@areveny areveny commented Jan 19, 2022

This change skips randomized size variation for the slicer toxic if the SizeVariation parameter is 0, which is also the default setting.

Additionally, it fixes a small bug in the randomization. rand.Intn(1) was being added to mid to adjust for integer division rounding. However, rand.Intn(n) returns an integer in the range [0, n), so rand.Intn(1) always returns 0. It has been changed to rand.Intn(2).

Closes #178

@miry miry self-assigned this Jan 19, 2022
toxics/slicer_test.go Outdated Show resolved Hide resolved
@areveny
Copy link
Contributor Author

areveny commented Jan 19, 2022

Removed the use of labels in the tests, also reduced the waiting time for timeout so the tests wait less long to complete. (But much more time than the delay so it won't time out unexpectedly.)

@miry
Copy link
Contributor

miry commented Jan 20, 2022

@areveny It looks like I still have problems random failing tests for Slicer.

$ MallocNanoZone=0 go test -v -race -timeout 1m ./...
...
=== RUN   TestSlicerToxic
    slicer_test.go:53: Expected to read about 480 times, but read 0 times.
    slicer_test.go:56: Server did not read correct buffer from client!
--- FAIL: TestSlicerToxic (0.02s)

@miry miry added this to the 2.4.0 milestone Jan 20, 2022
@miry miry added the Toxiproxy label Jan 20, 2022
@areveny
Copy link
Contributor Author

areveny commented Jan 20, 2022

Reverted the timeout to its original duration.

@miry
Copy link
Contributor

miry commented Jan 21, 2022

Thank you @areveny. Can you please update the title of the current PR and entry in CHANGELOG to make it more informative. Example: Handle toxic slicer when SizeVariation is null.
It would remove duplication in history for readers like: Fix bug.

@areveny areveny changed the title Fix slicer SizeVariation bugs Handle slicer Toxic with zero SizeVariation Jan 21, 2022
@areveny
Copy link
Contributor Author

areveny commented Jan 21, 2022

Great. All feedback so far has been incorporated.

toxics/slicer.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Strech Strech left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

I would suggest to take a look at the comment which might be outdated

toxics/slicer.go Outdated Show resolved Hide resolved
toxics/slicer.go Outdated Show resolved Hide resolved
@miry
Copy link
Contributor

miry commented Jan 24, 2022

@areveny Propose to change the code a bit. Moved the +{0,1} to default, according the comment it was connected to part (end-start)/2. Also removed the comment.

Copy link
Contributor

@miry miry left a comment

Choose a reason for hiding this comment

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

Some small refactorings

@areveny
Copy link
Contributor Author

areveny commented Jan 26, 2022

I have gone with the approach of removing the rand.Intn(2). The comment is removed as well.

Copy link
Contributor

@Strech Strech left a comment

Choose a reason for hiding this comment

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

Looks cleaner 👍🏼

@miry miry merged commit a00bea2 into Shopify:master Jan 26, 2022
@areveny areveny deleted the slicer-size-variation branch January 26, 2022 22:06
@miry miry mentioned this pull request Mar 3, 2022
8 tasks
@neufeldtech
Copy link
Contributor

Changes have been release as part of https://github.com/Shopify/toxiproxy/releases/tag/v2.4.0

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.

panic in slicer
4 participants