fix: clean up controller default name generation #282

metacosm · 2021-01-04T17:08:11Z

Fixes #279

s-soroosh · 2021-01-04T17:55:06Z

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java

@@ -6,6 +6,23 @@

 public interface ResourceController<R extends CustomResource> {

+  static String getDefaultNameFor(ResourceController controller) {


Wonder why these methods are needed to be here. Let's keep the interfaces as clean and relevant as possible!
I still believe the getName also should be pulled out from this interface, I think it's been missed in the main PR wrt quarkus extension.

We don't need to put these methods there, it just that it made sense to me to move them here since they pertain to ResourceControllers instead of adding them to a util class that's less discoverable.

@psycho-ir why do you think the getName method should be pulled? It's needed to be able to configure controllers individually.

It let the user set the controller name in 2 different ways, @Controller.name & overriding the getName method and that's what we should strive to avoid.

Ah yes, didn't think of that but I assume that someone would need to know what they're doing if they override the default getName method… since we provide a default implementation. And if they provide their own implementation, then I guess that's the one that would be used and the annotation would be ignored anyway. I don't think that's something that would be an issue in practice.

Well, API design is mainly about the things we let users do and the reason behind them.
The getName method has been implemented for internal use and is effectively final (no reason to be overridden by the client).

Having that in the interface as the method will lead to other questions like why finalizerName, crdName, generationAwareEventProcessing and so on don't have getter method in interface accordingly if there is a getter method with default implementation why @controller annotation is getting the name field too? and several similar questions.

The name is not internal: to be able to configure a controller, you need to be able to identify it. That's the name's purpose. While there are no obvious reasons that I can think of to override the default naming scheme, some people might still want to provide their own way to name a controller independently of what we provide (because for example, they're not using annotations and don't want to incur that impact at runtime). As such, it makes sense (at least to me) that it lives on the ResourceController interface.

The other methods, on the other hand, are part of the controller's configuration, which, again, you can't retrieve without first knowing the name of the controller. These values are not part of what is essential to the controller.

In comment, I was talking about the getName and not name, ofc name is not an internal thing, thus the user can set it via @Controller.name.

getName for sure can be added later on (it's backward compatible change) to the interface if it's been decided to get controller essential configuration from anywhere other than the controller annotation, AFAIK it's not currently the case and annotation + its crdName property is required.

metacosm · 2021-01-04T20:04:40Z

The change to use the fully-qualified name of the controller implementations is actually problematic and only works currently because of the fact that names are not being used consistently in all places. In particular, using FQNs doesn't work in application.yml files as dotted names are interpreted as composite paths instead of simple names. This is why the tests are currently failing with this new version of the code that tries to unify names.

This means that if we want to be able to configure a controller via a properties file, you need to add a name to the controller which doesn't seem like a good default behavior to me.

It seems like it would make more sense to require changing the default name as it was previously generated (i.e. using getSimpleName) only when there's a conflict so I'd actually be in favor of reverting that change. What do you think @adam-sandor?

The new naming algorithm makes it impossible to use the default name for to identify a controller in application.yaml files because FQNs are interpreted as composite paths instead of single names. This makes things more cumbersome, imo.

s-soroosh · 2021-01-04T22:04:40Z

The change to use the fully-qualified name of the controller implementations is actually problematic and only works currently because of the fact that names are not being used consistently in all places. In particular, using FQNs doesn't work in application.yml files as dotted names are interpreted as composite paths instead of simple names. This is why the tests are currently failing with this new version of the code that tries to unify names.

This means that if we want to be able to configure a controller via a properties file, you need to add a name to the controller which doesn't seem like a good default behavior to me.

It seems like it would make more sense to require changing the default name as it was previously generated (i.e. using getSimpleName) only when there's a conflict so I'd actually be in favor of reverting that change. What do you think @adam-sandor?

if the key has dots, then you just need to wrap the key with brackets, e.g:

controllers:
  '[io.javaoperatorsdk.operator.springboot.starter.testcontroller]':
    retry:
      maxAttempts: 3
      initialInterval: 1000
      intervalMultiplier: 1.5
      maxInterval: 50000
      maxElapsedTime: 100000

metacosm · 2021-01-05T06:58:11Z

Sure but can we expect most people to know that? Personally, I'd rather add a name to my ResourceController annotation than have to remember the syntax and have the risk of a typo in the potentially long package name.

s-soroosh · 2021-01-05T08:39:41Z

Sure but can we expect most people to know that? Personally, I'd rather add a name to my ResourceController annotation than have to remember the syntax and have the risk of a typo in the potentially long package name.

we need examples to illustrate how the controller can be configured via config files anyway, if one prefer to have shorter name for controller for sure it can be overridden with @Controller.name

metacosm · 2021-01-05T09:51:58Z

Sure but can we expect most people to know that? Personally, I'd rather add a name to my ResourceController annotation than have to remember the syntax and have the risk of a typo in the potentially long package name.

we need examples to illustrate how the controller can be configured via config files anyway, if one prefer to have shorter name for controller for sure it can be overridden with @Controller.name

Right but my point is that the default behavior should be as simple as possible. Right now, with this change, I feel we're making the default case more complex than it needs to be: how often will people have several controllers with the exact same simple name in their operators?

s-soroosh · 2021-01-05T10:00:48Z

Sure but can we expect most people to know that? Personally, I'd rather add a name to my ResourceController annotation than have to remember the syntax and have the risk of a typo in the potentially long package name.

we need examples to illustrate how the controller can be configured via config files anyway, if one prefer to have shorter name for controller for sure it can be overridden with @Controller.name

Right but my point is that the default behavior should be as simple as possible. Right now, with this change, I feel we're making the default case more complex than it needs to be: how often will people have several controllers with the exact same simple name in their operators?

I can't really say how often that can happen. however, it's already happening in the tests the SDK itself has.
it sounds more like concurrency issues, they don't happen so often but we don't really want even those few times it happens, so we need a way to prevent them and at the same time having a predictable behaviour for the user.

metacosm · 2021-01-05T10:20:18Z

Right but my point is that the default behavior should be as simple as possible. Right now, with this change, I feel we're making the default case more complex than it needs to be: how often will people have several controllers with the exact same simple name in their operators?

I can't really say how often that can happen. however, it's already happening in the tests the SDK itself has.
it sounds more like concurrency issues, they don't happen so often but we don't really want even those few times it happens, so we need a way to prevent them and at the same time having a predictable behaviour for the user.

Yes but I think that the fix should be to tell people to annotate their controller with name in that case, instead of making the default behavior more complex, and, of course, document this… 😄

s-soroosh · 2021-01-05T10:32:49Z

Right but my point is that the default behavior should be as simple as possible. Right now, with this change, I feel we're making the default case more complex than it needs to be: how often will people have several controllers with the exact same simple name in their operators?

I can't really say how often that can happen. however, it's already happening in the tests the SDK itself has.
it sounds more like concurrency issues, they don't happen so often but we don't really want even those few times it happens, so we need a way to prevent them and at the same time having a predictable behaviour for the user.

Yes but I think that the fix should be to tell people to annotate their controller with name in that case, instead of making the default behavior more complex, and, of course, document this… 😄

If we raise compiletime/runtime error in such cases, then it makes sense

metacosm · 2021-01-05T10:55:59Z

If we raise compiletime/runtime error in such cases, then it makes sense

I agree completely and I was planning on looking at implementing something to that effect. Just wanted to make sure we were on the same page regarding the default behavior first. What do you think @adam-sandor @csviri?

adam-sandor · 2021-01-05T14:32:44Z

I would be happy to keep the simple name if that's possible to implement

This allows to have consistent handling of duplicated controller names.

metacosm · 2021-01-06T21:25:28Z

Ready to merge.

adam-sandor · 2021-01-07T08:41:22Z

LGTM

fix: clean up controller default name generation

8bae9ab

Fixes #279

metacosm self-assigned this Jan 4, 2021

s-soroosh reviewed Jan 4, 2021

View reviewed changes

fix: tests

364066a

The new naming algorithm makes it impossible to use the default name for to identify a controller in application.yaml files because FQNs are interpreted as composite paths instead of single names. This makes things more cumbersome, imo.

feat: factor out ControllerConfiguration registration

d12bb06

This allows to have consistent handling of duplicated controller names.

metacosm force-pushed the controller-name-fix branch from 71f4fe1 to 85475e3 Compare January 5, 2021 17:29

feat: revert use of canonical class name as default controller name

cc819e1

metacosm force-pushed the controller-name-fix branch from 85475e3 to cc819e1 Compare January 5, 2021 17:31

metacosm added 2 commits January 6, 2021 21:59

refactor: move naming generation methods back to ControllerUtils

1a851ef

refactor: remove getName method from ResourceController

dc4b96f

adam-sandor merged commit 9e5cb13 into master Jan 7, 2021

metacosm deleted the controller-name-fix branch January 7, 2021 09:52

		@@ -6,6 +6,23 @@

		public interface ResourceController<R extends CustomResource> {

		static String getDefaultNameFor(ResourceController controller) {

fix: clean up controller default name generation #282

fix: clean up controller default name generation #282

Uh oh!

Conversation

metacosm commented Jan 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-soroosh Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metacosm commented Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-soroosh commented Jan 4, 2021

Uh oh!

metacosm commented Jan 5, 2021

Uh oh!

s-soroosh commented Jan 5, 2021

Uh oh!

metacosm commented Jan 5, 2021

Uh oh!

s-soroosh commented Jan 5, 2021

Uh oh!

metacosm commented Jan 5, 2021

Uh oh!

s-soroosh commented Jan 5, 2021

Uh oh!

metacosm commented Jan 5, 2021

Uh oh!

adam-sandor commented Jan 5, 2021

Uh oh!

metacosm commented Jan 6, 2021

Uh oh!

adam-sandor commented Jan 7, 2021

Uh oh!

Uh oh!

s-soroosh Jan 4, 2021 •

edited

Loading

metacosm commented Jan 4, 2021 •

edited

Loading