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 cache and persistence to registry #155

Closed
wants to merge 10 commits into from

Conversation

gabriel-indik
Copy link
Contributor

@gabriel-indik gabriel-indik commented Sep 13, 2024

Two items were discussed this morning:

  • Adding caching to the identity registry
  • Adding persistence to the identity registry

This PR addresses these items.

Signed-off-by: Gabriel Indik <[email protected]>
@gabriel-indik gabriel-indik self-assigned this Sep 13, 2024
Signed-off-by: Gabriel Indik <[email protected]>
@gabriel-indik gabriel-indik changed the title Add cache to registry Add cache and persistence to registry Sep 13, 2024
@gabriel-indik gabriel-indik marked this pull request as draft September 13, 2024 16:09
defer r.stateLock.Unlock()
return r.inMemoryPlaceholder[node]
re, _ := r.registryCache.Get(node)
return re
Copy link
Contributor

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.

Copy link
Contributor Author

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
}

Copy link
Contributor

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.

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.

2 participants