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

pkg/cachutil: Add test for AsyncOperationProcessor stop() behavior #8043

Closed
wants to merge 1 commit into from

Conversation

dsabsay
Copy link
Contributor

@dsabsay dsabsay commented Jan 7, 2025

The existing implementation sometimes drops existing operations that are still on the queue when .stop() is called.

If multiple communications in a select statement can proceed, one is chosen pseudo-randomly: https://go.dev/ref/spec#Select_statements

This means that sometimes a processor worker will process a remaining operation, and sometimes it won't.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

Will fix in a separate PR.

The existing implementation sometimes drops existing operations that are
still on the queue when .stop() is called.

If multiple communications in a select statement can proceed, one is
chosen pseudo-randomly: https://go.dev/ref/spec#Select_statements

This means that sometimes a processor worker will process a remaining
operation, and sometimes it won't.

Signed-off-by: Daniel Sabsay <[email protected]>
@dsabsay dsabsay mentioned this pull request Jan 7, 2025
2 tasks
@dsabsay
Copy link
Contributor Author

dsabsay commented Jan 7, 2025

Great. This test fails in CI as expected:

ok  	github.com/thanos-io/thanos/pkg/cache	1.305s
--- FAIL: TestAsyncOp (0.00s)
    testutil.go:91: async_op_test.go:32: ""
        
        	exp: 100
        
        	got: 58
        
FAIL
FAIL	github.com/thanos-io/thanos/pkg/cacheutil	0.181s

@dsabsay
Copy link
Contributor Author

dsabsay commented Jan 7, 2025

I will close this PR to prevent confusion. But the above shows that the test fails in CI. The fix (and test) is on #8044.

@dsabsay dsabsay closed this Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant