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 RLock for RemotelyControlledSampler.sampler Resolves #514 #515

Merged
merged 1 commit into from May 22, 2020
Merged

Conversation

hummerd
Copy link
Contributor

@hummerd hummerd commented May 20, 2020

Serialize read access for RemotelyControlledSampler.sampler field

Signed-off-by: Dima Kozlov hummerd@mail.ru

Which problem is this PR solving?

Serialize access to RemotelyControlledSampler.sampler

Short description of the changes

Wrap sampler with atomic.Value.

@yurishkuro
Copy link
Member

How about using a private struct samplerWrapper then?

type samplerWrapper struct {
  sampler Sampler
}

The concrete type stored in atomic.Value will be the same. Atomic value is much faster that full RW lock.

@hummerd
Copy link
Contributor Author

hummerd commented May 20, 2020

Actually RLock is one atomic atomic.AddInt32 operation, until there is no write Lock() and write locks will happen not too often in our case.

But let it be wrapper =)

Do we need sync.Lock in remote RemotelyControlledSampler after that?

@yurishkuro
Copy link
Member

You're right about RLock, let's keep it.

Please make sure the commits are signed.

Serialize read access for RemotelyControlledSampler.sampler field
Now sampler is atomic.Value holding reference to sampler

Signed-off-by: Dima Kozlov <hummerd@mail.ru>
@hummerd
Copy link
Contributor Author

hummerd commented May 21, 2020

It is ready. WIth RLock and sign-off.

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #515 into master will not change coverage.
The diff coverage is 50.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #515   +/-   ##
=======================================
  Coverage   88.75%   88.75%           
=======================================
  Files          60       60           
  Lines        3790     3790           
=======================================
  Hits         3364     3364           
  Misses        309      309           
  Partials      117      117           
Impacted Files Coverage Δ
sampler_remote.go 82.48% <50.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6853b8...426b60f. Read the comment docs.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@yurishkuro yurishkuro merged commit 880b3e9 into jaegertracing:master May 22, 2020
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

2 participants