-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Make BackedUpItems thread safe #8366
base: main
Are you sure you want to change the base?
Conversation
a9342af
to
ec5d953
Compare
Signed-off-by: Scott Seago <[email protected]>
ec5d953
to
f7f2402
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8366 +/- ##
==========================================
+ Coverage 58.95% 59.01% +0.06%
==========================================
Files 367 368 +1
Lines 38876 38907 +31
==========================================
+ Hits 22920 22962 +42
+ Misses 14493 14483 -10
+ Partials 1463 1462 -1 ☔ View full report in Codecov by Sentry. |
return resources | ||
} | ||
|
||
func (m *backedUpItemsMap) Size() int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Count()
instead of Size()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's only one map. This map has a size or length. No opinions.
return len(m.BackedUpItems) | ||
} | ||
|
||
func (m *backedUpItemsMap) ItemInBackup(key itemKey) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with the current names. But just thought of ItemInBackup
<-> IsItemBackedup
and AddItem
<-> MarkItemBackedup
|
||
// backedUpItemsMap keeps track of the items already backed up for the current Velero Backup | ||
type backedUpItemsMap struct { | ||
*sync.RWMutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we explore using sync.Map
instead of sync.RWMutex
?
https://pkg.go.dev/sync#Map
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is one better than the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT sync.Map
provides thread safe mechanism built into it. If we are using sync.RWMutex
then we ourself have to manage the concurrent access safely. sync.Map
handles the concurrent access internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like maps fits well for this usecase if we do not require type safety.
The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2) when multiple goroutines read, write, and overwrite entries for disjoint sets of keys. In these two cases, use of a Map may significantly reduce lock contention compared to a Go map paired with a separate Mutex or RWMutex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubham-pampattiwar I had not explored that yet, but it's worth looking into. From what @kaovilai pasted above, this may well be a better choice.
Thank you for contributing to Velero!
Please add a summary of your change
This PR synchronizes access to backupRequest.BackedUpItems with an RWMutex so that multiple ItemBlock processing goroutines will be able to access it.
Does your change fix a particular issue?
Fixes #7148
Please indicate you've done the following:
make new-changelog
) or comment/kind changelog-not-required
on this PR.site/content/docs/main
.