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

Fix multithreading issue in incremental cycle populator #366

Conversation

kpieprzak
Copy link
Contributor

@kpieprzak kpieprzak commented Jan 3, 2019

It is proposition of fix for issue #365

@PaulSandoz
Copy link
Contributor

Thank you for doing this. I question the approach to parallelism within HollowIncrementalCyclePopulator but we can revise that later on (perhaps using parallel streams that will likely have better characteristics with regards to load distribution, and/or thinking about the axis of parallelism, such as the type state thereby not requiring a thread safe bit set).

public class HollowIncrementalProducerMultithreadingTest {

private static final int ELEMENTS = 20000;
private static final double THREADS = 50.0d;
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC running on a machine with say 32 hardware threads will potentially create a lot of threads for not much benefit as only 32 threads will be running.

Can you please reduce to say 4 (that is the degree of over-provisioning fork join does to compensate for lumpy loads) but run each test within a loop for a number of iterations, thereby increasing the likely hood that a race condition will be encountered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@PaulSandoz PaulSandoz added the bug label Jan 11, 2019
Copy link
Contributor

@dkoszewnik dkoszewnik left a comment

Choose a reason for hiding this comment

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

Thank you very much @kpieprzak

@PaulSandoz PaulSandoz merged commit 1f402fa into Netflix:master Jan 14, 2019
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.

3 participants