-
Notifications
You must be signed in to change notification settings - Fork 12
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 cache and persistence to registry #155
Conversation
Signed-off-by: Gabriel Indik <[email protected]>
Signed-off-by: Gabriel Indik <[email protected]>
Signed-off-by: Gabriel Indik <[email protected]>
Signed-off-by: Gabriel Indik <[email protected]>
Signed-off-by: Gabriel Indik <[email protected]>
defer r.stateLock.Unlock() | ||
return r.inMemoryPlaceholder[node] | ||
re, _ := r.registryCache.Get(node) | ||
return re |
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.
To prevent unintended changes to the cached data, this function should return a list of objects after RegistryNodeTransportEntry objects after running a deepCopy. Otherwise, the caller can make changes and that will change the cached data.
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.
Thank you for the feedback, great point.
Would this be a good way to resolve the issue @dwertent:
func (r *registry) getNodeTransports(node string) []*components.RegistryNodeTransportEntry {
re, _ := r.registryCache.Get(node)
cpy := make([]*components.RegistryNodeTransportEntry, len(re))
copy(cpy, re)
return cpy
}
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 don't think that's the case, as the copy
function performs a shallow copy by default. You can verify this with a simple test like the following:
r.UpsertTransportDetails(...)
n1 := r.getNodeTransports(<node>)
n2 := r.registryCache.Get(node)
for i := range n1 {
assert.NotSame(t, n1[i], n2[i]) // compare pointers
}
To fix this, you can either implement a DeepCopy
function, like so:
func (r *registry) getNodeTransports(node string) []*components.RegistryNodeTransportEntry {
re, _ := r.registryCache.Get(node)
cpy := make([]*components.RegistryNodeTransportEntry, len(re))
for i, reg := range re {
cpy[i] = reg.DeepCopy() // this would need to be implemented
}
return cpy
}
Alternatively, you could use a shallow copy method for structs without pointers:
func (r *registry) getNodeTransports(node string) []*components.RegistryNodeTransportEntry {
re, _ := r.registryCache.Get(node)
cpy := make([]*components.RegistryNodeTransportEntry, len(re))
for i, reg := range re {
tmp := *reg
cpy[i] = &tmp
}
return cpy
}
However, the second solution won't work if RegistryNodeTransportEntry
contains fields with pointers.
Two items were discussed this morning:
This PR addresses these items.