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

logging: log maximum concurrency level when talking to backend storage #1629

Merged
merged 1 commit into from Dec 29, 2021

Conversation

jkowalski
Copy link
Contributor

Trying to understand if we're doing something wrong for #1560

@codecov
Copy link

codecov bot commented Dec 29, 2021

Codecov Report

Merging #1629 (8c354a8) into master (13ed6bb) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1629      +/-   ##
==========================================
+ Coverage   70.12%   70.23%   +0.11%     
==========================================
  Files         381      381              
  Lines       30215    30240      +25     
==========================================
+ Hits        21187    21238      +51     
+ Misses       7388     7370      -18     
+ Partials     1640     1632       -8     
Impacted Files Coverage Δ
repo/blob/logging/logging_storage.go 100.00% <100.00%> (ø)
repo/content/content_manager.go 84.76% <0.00%> (-0.58%) ⬇️
internal/epoch/epoch_manager.go 90.60% <0.00%> (+0.52%) ⬆️
internal/server/server.go 67.94% <0.00%> (+0.64%) ⬆️
repo/maintenance/maintenance_run.go 75.94% <0.00%> (+0.86%) ⬆️
repo/object/object_reader.go 78.53% <0.00%> (+1.46%) ⬆️
repo/content/content_manager_lock_free.go 73.88% <0.00%> (+2.23%) ⬆️
repo/maintenance/maintenance_schedule.go 74.13% <0.00%> (+2.58%) ⬆️
internal/ownwrites/ownwrites.go 81.48% <0.00%> (+2.77%) ⬆️
internal/listcache/listcache.go 83.33% <0.00%> (+3.92%) ⬆️
... and 1 more

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 13ed6bb...8c354a8. Read the comment docs.

@jkowalski jkowalski merged commit 0149538 into kopia:master Dec 29, 2021
Copy link
Collaborator

@julio-lopez julio-lopez left a comment

Choose a reason for hiding this comment

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

@jkowalski PTAL

Comment on lines +26 to +28
if atomic.CompareAndSwapInt32(&s.maxConcurrency, mv, v) {
s.logger.Debugw(s.prefix+"concurrency level reached",
"maxConcurrency", mv)
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor:

Is there a small race here?

Suppose:

  • the state is concurrency == maxConcurrency, for example 7;
  • there are two routines g1 and g2 concurrently running beginConcurrency;
  • g1 gets v == 8 and g2 gets v == 9;
  • both g1 and g2 get mv == 7 and both attempt to update maxConcurrency;
  • g1's update succeeds and now maxConcurrency == 8, but concurrency > maxConcurrency

This may self correct if there is a higher concurrency value down the road.

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