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

core/aggsigdb: refactor concurrent access issue in test #1950

Merged
merged 5 commits into from
Mar 28, 2023

Conversation

gsora
Copy link
Collaborator

@gsora gsora commented Mar 27, 2023

Use sync.Map with a thin abstraction for data and keys by duty maps.

category: bug
ticket: #1867

Use sync.Map with a thin abstraction for data and keys by duty maps.
@gsora gsora added the bug Something isn't working label Mar 27, 2023
@gsora gsora requested a review from corverroos March 27, 2023 17:41
@gsora gsora self-assigned this Mar 27, 2023
@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@f88ef52). Click here to learn what that means.
Patch coverage: 87.75% of modified lines in pull request are covered.

❗ Current head 1825cc8 differs from pull request most recent head 8e0f37d. Consider uploading reports for the commit 8e0f37d to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1950   +/-   ##
=======================================
  Coverage        ?   55.45%           
=======================================
  Files           ?      174           
  Lines           ?    22344           
  Branches        ?        0           
=======================================
  Hits            ?    12391           
  Misses          ?     8373           
  Partials        ?     1580           
Impacted Files Coverage Δ
core/aggsigdb/memory.go 84.57% <87.75%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

This way we synchronize based off map's interaction.
Copy link
Contributor

@corverroos corverroos left a comment

Choose a reason for hiding this comment

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

aggsigdb has a central goroutine that accesses all state, so sycnronisation not required. If race is due to tests, rather use callback functions called by that central goroutine.

@gsora
Copy link
Collaborator Author

gsora commented Mar 28, 2023

Will fix the test instead.

@gsora gsora closed this Mar 28, 2023
@gsora gsora reopened this Mar 28, 2023
@gsora
Copy link
Collaborator Author

gsora commented Mar 28, 2023

Reopened with a new approach.

LMK what you think @corverroos.

@corverroos corverroos changed the title core/aggsigdb: refactor to sync.Map core/aggsigdb: refactor concurrent access issue in test Mar 28, 2023
@gsora gsora added the merge when ready Indicates bulldozer bot may merge when all checks pass label Mar 28, 2023
@obol-bulldozer obol-bulldozer bot merged commit e5a1e00 into main Mar 28, 2023
@obol-bulldozer obol-bulldozer bot deleted the gsora/aggsigdb_race branch March 28, 2023 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants