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

Help with cache.NewSnapshot breaking change and outdated documentation #513

Closed
ekristen opened this issue Nov 15, 2021 · 4 comments · Fixed by #520
Closed

Help with cache.NewSnapshot breaking change and outdated documentation #513

ekristen opened this issue Nov 15, 2021 · 4 comments · Fixed by #520

Comments

@ekristen
Copy link

I'm currently on 0.9.9 and looking at going to 0.10.0 and it looks like a breaking change was introduced. It also appears that this change was accidentally omitted from the changelog, unless I missed it?

It appears that the cache.NewSnapshot function was changed to take a different set of arguments however all existing documentation still refers to the old function arguments.

Can anyone help with an upgrade example?

@nfuden
Copy link

nfuden commented Nov 15, 2021

Per the test here:

func TestSnapshotWithOnlyEndpointIsInconsistent(t *testing.T) {

and the list of resource types that it validates here:

func GetResponseType(typeURL resource.Type) types.ResponseType {

You could go from
snapshot := cache.NewSnapshot("1.0", endpoints, clusters, routes, listeners, runtimes)
To
snapshot := cache.NewSnapshot("1.0", map[resource.Type][]types.Resource{ resource.EndpointType: {endpoints}, resource.ClusterType: {clusters}, resource. RouteType: {routes}, resource. ListenerType: {listeners}, resource. RuntimeType: {runtimes}, })

and one of the nice things about the signature change (other than extensibility) is that if you only care about a subsection of the old arguments you no longer have to specify all the slices in this map.

Take the above comment with many grains of salt as I have not tested/used 0.10.0 and inspected the test suite to derive this.

@ekristen
Copy link
Author

Thanks @nfuden I'll have to give that a try. I looked at the tests but it was a bit confusing. Mainly trying to understand the map[string] bit first, guess maybe that's making room for expansion in the future any that's why there is only a single entry resource.Type?

I'll give this a shot.

@alecholmez
Copy link
Contributor

Hi there! I will update the documentation, it is old and needs to be brought up, we have changed quite a bit since that was written. Sorry for the inconvenience I will try and knock that out tomorrow

@alecholmez
Copy link
Contributor

alecholmez commented Dec 3, 2021

Hi there! Just opened this PR to address some of these issues: #520.

As per earlier comments @nfuden was on the money. The purpose of the signature change was to aid with the addition of xDS services as upstream defines more. We had to modify the signature every time a new service was created. That is no longer the case plus this provides some flexibility around resources we specify, as well as aids in support for generic/opaque resource type support.

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 a pull request may close this issue.

3 participants