Skip to content

Remove ResourceDiscriminators (legacy approach) #2253

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

Closed
csviri opened this issue Feb 27, 2024 · 6 comments · Fixed by #2299
Closed

Remove ResourceDiscriminators (legacy approach) #2253

csviri opened this issue Feb 27, 2024 · 6 comments · Fixed by #2299
Milestone

Comments

@csviri
Copy link
Collaborator

csviri commented Feb 27, 2024

With this PR: #2252

we can handle this quite elegantly without the resource discriminators, is there are use case (also for external resources) when we need this former approach anymore? If we cannot think we should consider removing this for v5

@csviri
Copy link
Collaborator Author

csviri commented Feb 28, 2024

The only case with is not handled now elegantly is the read-only dependent resource since that does not have the desired state by default. There are multiple choices regarding this:

  • the users will have still to provide the desired for read-only but just with the metadata only. Maybe we can make this nicer, providing an API to override that just returns a ResourceID.
  • we keep the discriminators
  • the read only is resource is still a corner case, to by just overriding the DependentResource.getSecondaryResource the user can provide it's own implementation.

@metacosm
Copy link
Collaborator

I think we should keep them for optimization purposes as well. If the desired state is costly to compute and there are better ways to decided which secondary resource needs to be used, I think it still makes sense to use a discriminator even if it shouldn't be needed in most cases. That said, we could indeed tell people to override getSecondaryResource in that situation.

@csviri
Copy link
Collaborator Author

csviri commented Feb 28, 2024

indeed tell people to override getSecondaryResource in that situation

exactly, basically resource discriminator puts nothing on table now, as we discussed before the only reason to create was to have a class representing the concept explicitly, so it is settable explicitly on a resource. But it's API is basically just the context:

Optional<R> distinguish(Class<R> resource, P primary, Context<P> context);

On the other hand desired needs to be always computed, except in the read-only (what is already kinda exotic case https://javaoperatorsdk.io/docs/dependent-resources#read-only-dependent-resources-vs-event-source )

But will examine the problem more in depth.

@metacosm
Copy link
Collaborator

metacosm commented Feb 28, 2024

On the other hand desired needs to be always computed, except in the read-only (what is already kinda exotic case https://javaoperatorsdk.io/docs/dependent-resources#read-only-dependent-resources-vs-event-source )

Yes but if it's costly to compute, we should try to avoid computing it several times… maybe we should record the computed desired state(s) in the Context so that they are available for downstream usage once they have been computed once?

@csviri
Copy link
Collaborator Author

csviri commented Feb 28, 2024

yes definitelly, that is something will be done even if the discriminators are not removed.

@Javatar81
Copy link

After using JOSDK for quite some time now, I think we only need discriminators for dependents because dependents currently have no identifiers. This is why @csviri proposes to evaluate the resource that the desired method returns.

To overcome this problem of missing identifier, I've added the following method to all of my dependent classes public static String getName(P primary) where P is the primary CRD type. This allows me to derive the name from the name of the primary in a consistent way throughout my code, e.g. I call getName in every discriminator and I am able to find the Kubernetes via fabric8 client even without the context (e.g. useful for test cases).

You could force users to define the identifier upfront (e.g. by an abstract method ResourceID getId(P primary, Context<P> context) that has to be implemented). This has several benefits:

  1. You would have a way to determine the resource id even without calling desired
  2. You would not need discriminators because you can rely on the getId method to distinguish resources of the same type
  3. You could even enforce that the resource returned by desired always has the name/namespace returned by getId (today it is possible that I change the name of the resource between multiple calls of the desired method)

@csviri csviri linked a pull request Mar 20, 2024 that will close this issue
@csviri csviri closed this as completed Mar 25, 2024
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