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

Issue #448 make state thread safe in consumer_partition and connection #451

Merged
merged 4 commits into from
Jan 26, 2021

Conversation

zzzming
Copy link
Contributor

@zzzming zzzming commented Jan 25, 2021

Motivation

Address Issue: #448 and connection.go also missing a few sync against state

Modifications

Use go sync/atomic to synchronize state and add to missing ones in connection.go

I did not use uber's atomic as suggested by @flowchartsman. The sync/atomic seems adequate and avoid another dependency. If uber's atomic is required, we should update other sync/atomic to be consistent. @merlimat @wolfstudy what do you think? thanks

Verifying this change

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented?
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@flowchartsman
Copy link
Contributor

I'm not particularly attached to uber's atomic, I used it for expediency's sake, but direct calls to atomic always make me nervous if I'm doing them from multiple places, so guarding it in a (probably inlined) method is usually the way I like to go. YMMV. But, if it works and it's consistent with other code, who am I to judge? ¯_ʕ◔ϖ◔ʔ_/¯ Aside from that, LGTM

@zzzming
Copy link
Contributor Author

zzzming commented Jan 25, 2021

I'm not particularly attached to uber's atomic, I used it for expediency's sake, but direct calls to atomic always make me nervous if I'm doing them from multiple places, so guarding it in a (probably inlined) method is usually the way I like to go. YMMV. But, if it works and it's consistent with other code, who am I to judge? ¯_ʕ◔ϖ◔ʔ_/¯ Aside from that, LGTM

@flowchartsman I share the same sentiment that I would tend to forget about add sync.atomic too. Let's wait for Matteo to weight in. It's a minor change either way. cheers

@merlimat
Copy link
Contributor

The uber/atomic looks indeed more readable and safer than the sync/atomic. It would be good to use that consistently

@zzzming
Copy link
Contributor Author

zzzming commented Jan 26, 2021

The uber/atomic looks indeed more readable and safer than the sync/atomic. It would be good to use that consistently

updated all states with uber/atomic

@merlimat merlimat added this to the 0.4.0 milestone Jan 26, 2021
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