[#221] pool: Make sampler safe for concurrent using #222

dkirillov · 2024-05-08T11:56:01Z

dkirillov commented

2024-05-08 11:56:01 +00:00

close #221

Signed-off-by: Denis Kirillov d.kirillov@yadro.com

close #221 Signed-off-by: Denis Kirillov <d.kirillov@yadro.com>

dkirillov self-assigned this 2024-05-08 11:56:02 +00:00

dkirillov requested review from storage-core-committers 2024-05-08 11:56:17 +00:00

dkirillov requested review from storage-core-developers 2024-05-08 11:56:19 +00:00

dkirillov requested review from storage-sdk-committers 2024-05-08 11:56:22 +00:00

dkirillov requested review from storage-sdk-developers 2024-05-08 11:56:29 +00:00

dkirillov requested review from storage-services-committers 2024-05-08 11:56:32 +00:00

dkirillov requested review from storage-services-developers 2024-05-08 11:56:32 +00:00

fyrchik reviewed 2024-05-08 12:00:00 +00:00

pool/sampler_test.go Outdated

					
				@ -40,6 +41,28 @@ func TestSamplerStability(t *testing.T) {

					}

				}

				func TestConcurrentSampler(t *testing.T) {

fyrchik commented

2024-05-08 11:59:57 +00:00

How long does it take for this test to execute?
If it triggers panic, we could use 2 goroutines and -race flag instead, should fail too.

How long does it take for this test to execute? If it triggers panic, we could use 2 goroutines and `-race` flag instead, should fail too.

dkirillov commented

2024-05-08 12:34:42 +00:00

--- PASS: TestConcurrentSampler (0.82s)

``` --- PASS: TestConcurrentSampler (0.82s) ```

dkirillov commented

2024-05-08 12:37:23 +00:00

If it triggers panic, we could use 2 goroutines and -race flag instead, should fail too.

Yes, we can add -race to test makefile target

> If it triggers panic, we could use 2 goroutines and -race flag instead, should fail too. Yes, we can add `-race` to `test` makefile target

fyrchik commented

2024-05-08 13:04:32 +00:00

No, I mean the test itself is not worth having IMO:

-race should catch the race even with 2 concurrent threads, 10_000 threads create a noticeable increase in test running time.
We have lots of other places that use mutexes but don't have similar tests, changes for sampler are trivial enough IMO, it has 1 public method and 1 field we need to protect.

No, I mean the test itself is not worth having IMO: 1. `-race` should catch the race even with 2 concurrent threads, 10_000 threads create a noticeable increase in test running time. 2. We have _lots_ of other places that use mutexes but don't have similar tests, changes for `sampler` are trivial enough IMO, it has 1 public method and 1 field we need to protect.