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

Replace usage of sync/atomic with uber-go/atomic #2449

Merged
merged 5 commits into from
Jul 30, 2020

Conversation

jvrplmlmn
Copy link
Contributor

What this PR does / why we need it: 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:
Partially addresses #2448

Special notes for your reviewer: This only updates the code to use the alternative package. I haven't added any automation/linter to make sure that this is enforced.
For that, we could use faillint, but I am not 100% sure of where this project is handling the installation of additional tools. Is that on loki-build-image/Dockerfile?

Checklist

  • Documentation added
  • Tests updated

Signed-off-by: Javier Palomo <javier.palomo.almena@gmail.com>
Signed-off-by: Javier Palomo <javier.palomo.almena@gmail.com>
Signed-off-by: Javier Palomo <javier.palomo.almena@gmail.com>
@CLAassistant
Copy link

CLAassistant commented Jul 30, 2020

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2020

Codecov Report

Merging #2449 into master will decrease coverage by 0.13%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2449      +/-   ##
==========================================
- Coverage   62.91%   62.77%   -0.14%     
==========================================
  Files         162      162              
  Lines       13952    13952              
==========================================
- Hits         8778     8759      -19     
- Misses       4491     4510      +19     
  Partials      683      683              
Impacted Files Coverage Δ
pkg/distributor/distributor.go 78.80% <60.00%> (ø)
pkg/ingester/mapper.go 90.36% <100.00%> (ø)
pkg/promtail/positions/positions.go 46.49% <0.00%> (-13.16%) ⬇️
pkg/promtail/targets/file/tailer.go 76.13% <0.00%> (-2.28%) ⬇️
pkg/logql/evaluator.go 92.47% <0.00%> (-0.41%) ⬇️

@owen-d
Copy link
Member

owen-d commented Jul 30, 2020

Hey, thanks for this -- it looks great. I agree using faillint will help maintain these changes in the future.

Feel free to installl it here: https://github.com/grafana/loki/blob/master/loki-build-image/Dockerfile#L10-L12

and then reference in the make lint target here https://github.com/grafana/loki/blob/master/Makefile#L241

Copy link
Contributor

@sandeepsukhani sandeepsukhani 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 for the PR!
Regarding faillint, you are right about using loki-build-image/Dockerfile for setting up additional tools.
You can check PR in Cortex for reference. cortexproject/cortex#2272

Signed-off-by: Javier Palomo <javier.palomo.almena@gmail.com>
Signed-off-by: Javier Palomo <javier.palomo.almena@gmail.com>
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jul 30, 2020
@jvrplmlmn
Copy link
Contributor Author

Thanks for the pointers @owen-d @sandeepsukhani.

It was not possible to add it to the golangci build image because it doesn't have the go toolchain in order to install the binary, so I created its own step.

I am not that familiar with the structure of this particular Makefile so let me know if the lint target was the right place to add the linter.

Copy link
Member

@owen-d owen-d 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!

@owen-d owen-d merged commit 6852b1c into grafana:master Jul 30, 2020
@jvrplmlmn jvrplmlmn deleted the use-ubergo-atomic branch July 30, 2020 18:13
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.

None yet

5 participants