Skip to content

Conversation

@jvrplmlmn
Copy link
Contributor

What this PR does: Replaces all direct usages of sync/atomic with go.uber.org/atomic. That way we ensure that all access to variables that have atomic access is in fact always atomic.

Which issue(s) this PR fixes:
Fixes #2923

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@jvrplmlmn jvrplmlmn force-pushed the use-ubergo-atomic branch from 23541c5 to 8ea70fb Compare July 29, 2020 19:32
@jvrplmlmn
Copy link
Contributor Author

@pracucci would you mind reviewing this PR?

@pstibrany
Copy link
Contributor

Thanks for doing this! Would you mind looking at https://github.com/grafana/loki repository as well? Some of the code there is copied from Cortex.

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Fantastic job, well done, thanks! ❤️ 👏

@pstibrany Could you review this PR too, so we can get this merged?

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot!

There are some places where we should remove pointers to atomic types, and use them directly:

  • pkg/ingester/ingester_v2.go:57
  • pkg/querier/frontend/frontend.go:70
  • pkg/querier/frontend/worker_frontend_manager.go:37
  • pkg/ring/lifecycler.go:125
  • pkg/ring/kv/multi.go:92
  • pkg/ring/kv/multi.go:98

Do you think we can get it done as part of this PR?

@jvrplmlmn
Copy link
Contributor Author

Hey @pstibrany, I would rather have this merged as it is and send those additional changes in a subsequent PR if that is okay with you.

@pstibrany
Copy link
Contributor

Hey @pstibrany, I would rather have this merged as it is and send those additional changes in a subsequent PR if that is okay with you.

Sounds good to me.

@pstibrany pstibrany merged commit e95eaec into cortexproject:master Jul 30, 2020
@pstibrany
Copy link
Contributor

Thanks for your PR and looking forward to the next one ;-)

@jvrplmlmn jvrplmlmn deleted the use-ubergo-atomic branch July 30, 2020 12:19
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.

Use uber-go/atomic in place of sync/atomic

3 participants