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

Add synchronization around use of @cluster and other variables Fix #332 #333

Merged
merged 2 commits into from Oct 18, 2016
Merged

Add synchronization around use of @cluster and other variables Fix #332 #333

merged 2 commits into from Oct 18, 2016

Conversation

hydrogen18
Copy link
Contributor

This class has the correct primitives to be thread safe, but it was missing a synchronize block.
Fix #332

@pezra
Copy link
Contributor

pezra commented Oct 17, 2016

Please rebase and i'll merge this.

@hydrogen18
Copy link
Contributor Author

This has no conflicts now.

@pezra pezra merged commit cfa015f into cequel:master Oct 18, 2016
@pezra
Copy link
Contributor

pezra commented Oct 18, 2016

Thanks for the improvement.

@bernardobarreto
Copy link

bernardobarreto commented Dec 5, 2016

Hi @pezra, @hydrogen18.
Could you guys, explain why I'm getting this error sometimes and what does it mean?:
screen shot 2016-12-05 at 16 15 06

Thanks in advance.

@pezra
Copy link
Contributor

pezra commented Dec 23, 2016

@bernardofire, that is pretty interesting. Given the if defined? @cluster guard it implies that multiple threads are running this method at the same time. Cequel is not thread safe, though we are working in that direction. Are you running this in a multi-threaded setup?

@lxbrito
Copy link

lxbrito commented Jan 2, 2017

Hi @pezra,
@bernardofire asked me to answer you. Yes, definitely. We are running some instances of Sidekiq with 12 threads each. Do you think that doubling the sidekiq instances while halving the thread count would make us see less @cluster errors?

@pezra
Copy link
Contributor

pezra commented Jan 5, 2017

I suspect you would see less of that issue with more processes and fewer threads.

Another option would be to submit a PR to convert that variable into a thread variable or synchronize access to that variable. I would love to get Cequel fully thread safe. If you choose this route I will do everything i can to support your effort.

@hydrogen18
Copy link
Contributor Author

@lxbrito @bernardofire You're using 2.0.2 which does not contain the fix for this issue, upgrade to 2.0.3

@lxbrito
Copy link

lxbrito commented Jan 5, 2017

@hydrogen18 Do you mean git master? I couldn't find 2.0.3 release.

@pezra
Copy link
Contributor

pezra commented Jan 5, 2017

I haven't released 2.0.3 yet. I am getting a couple more tiny PRs in. It should be out today.

@pezra
Copy link
Contributor

pezra commented Jan 6, 2017

Released in 2.0.3. Thanks, again, for the improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants