Add a priority concept to components

Development
#1

Hi everyone,

so that proposal is about adding a new concept of Priority in the Component descriptor interface. This priority would have different usages:

  1. if 2 components have same hint and role, the one with most priority would override the other one (this would solve: [XCOMMONS-1814] Component with higher priority is not taking precedence when deployed as an extension - XWiki.org JIRA). This would need to be backward compatible with the priorities defined in components.txt
  2. when getting list or map of components through the API, those list and maps would be automatically ordered by priority: this would allow different UC, like some transformation happening before others, or some listener being called before others

For defining the priority in Java components there would be different mechanism:

  1. for backward compatibility reason, it should still be possible to define it through components.txt (which would have the priority over other mechanisms)
  2. it should be possible to also define the priority when defining a new component as a Java class, so I propose to add the priority field in the Component annotation
  3. I don’t know much the definition of components through xobject, but we should also find a way to define the priority there (probably through a new property)

For backward compatibility reason, all components which do not define their priority would have a same priority value (probably defined to 1000 as it’s the actual default value in components.txt if I’m correct).

Now I also propose that contrarily to what we have now, we do define a specific ordering of the components when they all share the same priority, to stop relying on randomness: so I propose that we use lexicographic sorting based on the fully qualified name of the components for that.

#2

You don’t mention what your proposal means for ComponentDescriptor, but that’s what everything is based on.

For me, this does not really make sense.

There should be two completely different values since it’s two completely different use cases and needs and I can easily think of use cases where you need to override a specific component and at the same time order it among others components which share the same role and there is very little chance that you can use the same value for both (just think of a use case where you override a component to change its order).

If you really want to talk about both subjects in the same proposal, I would suggest the following changes:

  • ComponentDescriptor should have two different getters for those concepts (“override priority” and “role priority”)
  • As you proposed, having the “role priority” as a field of the @Component annotation sounds good
  • Keeping components.txt as the way to configure the “override priority” for Java components is probably enough, I’m fine having the possibility to also configure it at @Component level as long as it’s a different field
  • For wiki based components, all it takes is adding two properties for those concepts in whatever xclass which is used to define them
#3

Indeed, I was more focused on the ordering UC and I mixed with the other ones because I found it when searching for jira tickets, and was using the same “priority” vocabulary. But yes, I agree they should be considered as two different things.

I’m fine with override priority but I think I would use ordering priority for the second one.

I think I would not put it in the annotation: AFAIU it’s not a requirement for fixing [XCOMMONS-1814] Component with higher priority is not taking precedence when deployed as an extension - XWiki.org JIRA.

+1 with the other changes

#4

Actually, after thinking about it a bit more, I would suggest concept naming a little more consistent with what we currently have in the components API:

  • “role type priority” (what is used to order components sharing the same role type)
  • “role hint priority” (what is used to decide which of the components sharing the same role hint should be selected)

So that in ComponentDescriptor/ComponentRole you end up with:

  • getRoleType()
  • getRoleTypePriority()
  • getRoleHint()
  • getRoleHintPriority()
#5

Hi,

We already have a concept of priority for components (through components.txt), as registration time (Component Module (XWiki.org)). It seems this proposal is about something else:

  • Regarding point 1), I consider that a bug. Right now we only look at the component priority during the initial scan and registration of components, and not during dynamic registration of components. Right now when we register a new component dynamically, it always replaces the existing component role/hint. We could decide to look at the priority (thus save it since right now it’s not part of the CD).

  • Regarding point 2), this is a different topic and it’s not related to priority for the same role/hint. It’s a priority only for roles.

Some general thoughts:

You didn’t mention if this proposal has any impact on CM performance. It’s very important that it doesn’t as CM is a critical piece of code and is used a lot, and we need to keep it performant. For example if you do the sorting when registering a new component then that’s going to impact perfs a lot.

Small note that our original idea was to make the simplest possible CM with the least amount of features, so that we could later dump our CM impl and use an existing impl (like CDI, Guice, OSGi, etc). So the more features we add, the harder it is to find an existing component system that has these features.

Now, in practice, it’s been over 15 years that XWiki has its own CM impl and I’m doubting that we would we able to switch to another impl. It would be interesting though to see if there are CM impl nowadays that support dynamic registration of components (that used to not be supported at the time by all existing CMs except OSGi which was too complex).

Sounds good globally (but only if general performances are not impacted, we’ll need a perf benchmark to verify this).

Thanks!

#6

So as Thomas noticed in Add a priority concept to components - #2 by tmortagne I mixed two different things in the same proposal on the first post, because of the sharing of “priority” vocabulary. But indeed there’s two different things:

The idea would be to avoid the performance impact as much as possible. I didn’t mention it, because for me it was obvious that we need to be careful about it.
I actually already started to request opinion on this particular point in the draft PR: https://github.com/xwiki/xwiki-commons/pull/265#discussion_r959567118

#7

So Thomas actually found back an old proposal about using a @Priority annotation, which has been implemented and which is used for the mandatory documents apparently. So I now propose to reuse the same annotation here.

#8

yes obviously but for me it must not have any performance degradation for the main paths (getInstance()). That’s very important since the CM is used a lot.

#9

In that thread we didn’t fully agree about using that annotation vs using dedicated XWiki priority annotations. I have the same comments I had back then.

#10

Yes so I’m reading the thread and it looks like the problem was about (again) the possible usage of various kind of priorities.
Now the thing is the javax.annotation.Priority annotation is used for ordering the MandatoryDocumentInitializer components after retrieving them through ComponentManager#getComponentList.
My proposal here would be only to extend that behaviour to all other components. So basically any component could have a @Priority annotation, and that would be used for ordering the components in the CM.

If a specific kind of component needs a priority for another UC than “getting an ordering list of components from the CM”, then it would need to use another way to express it.

#11

Reviving this thread so that we agree on the remaining questions to continue working on the work started by @surli in the PR:

  1. Should we reuse MandatoryDocumentInitializer's specification (the role type priority is controlled by javax.annotation.Priority) or introduce a more specific metadata (like the new field in @Component annotation initially proposed) ?

  2. Should the role type priority be applied cross namespaces or not ? For example, should EventListener components registered at root level always be listed after (or before ?) those registered at “wiki:xwiki” level or should they all be ordered according to their priority no matter from which namespace they are coming from ?

My vote goes to:

  1. javax.annotation.Priority but I don’t mind using something else if others insist on it
  2. I feel the expectation will be that the role type priority should be applied no matter from which namespace the component is coming from. In the listener use case if I have a listener in XWiki Standard that I really want to be called as early as possible because it could have an important impact on following ones it does not make sense to me that some extension listener without any specific priority end up being call before just because it’s in an extension installed in a wiki.
#12

+1 for the reuse. At least I fail to see where it can be an issue.

That’s a tough one.
I tend to think that, at least by default, the priorities should be isolated per namespace.
It probably depends of the use case(s) that lead to this proposal.

Additional question:
Did we plan to allow a component to inject another component with the same role and hint of lower priority?
I think to could be useful, for instance to override a component while still being able to reuse its instance.

#13

I liked your proposal with “role type priority” and “role hint priority” more, but since we already use javax.annotation.Priority then I guess it’s better to be consistent. So +1 for this.

+1 for cross namespace. When developing a component, most of the time we don’t care in which namespace that component will be installed. So I don’t think we want to limit the priority for the namespace where the component is installed.

#14

Actually, there was another question, regarding role hint priority this time:

  1. Should the role hint priority be applied cross namespace or not: if two components with same role type and hint are registered at root level for one and “wiki:xwiki” for the other, but the one at root level have a higher priority, which one should win ?

This one is IMO more debatable than for the same question regarding role type priority, but I still feel that being in a more specific namespace is not enough to totally ignore the priority.

#15

+1 to reuse javax.annotation.Priority

Feels like it would be the easiest archi to always apply the priority cross namespace, so +1 for this

and +1 to have priority cross namespace here too, as I think it would add a lot more complexity (not necessarily to implement it, but to debug and understand how component priority is working) if we have another mechanism there

#16

After re-reading the entire discussion, and against my initial vote, I’m +1 to make the order cross namespaces, for both types of priorities.

#17

Just to make sure I understand this thread :slight_smile:

  • Override priority - same role, same hint. The concept already exists. UC: Loading.... We need EM to take into account the existing override priority (when installing an extension having components) and for that we need to have the override priority defined in Java (ie in the ComponentDescriptor) when dynamically registering a component in Java so that the CM doesn’t blindly overwrite any existing component (it would do that only if the override priority is higher than the existing component). Correct?

  • Role priority - same role, different hints. For the list/map UC (e.g. Macros). Do we have an existing use case for this? Do we have a jira of a problem to solve? I’m not saying I don’t agree, just curious if it’s something nice to have for the future or if we have a need.

Note: It took me a long time to understand how to read this. I actually understood it backward initially… So not sure, it’s the most understandable API name.

Just to make sure I get it :slight_smile: This is a priority when you have 2 components with the same role type, independently of the hint value (actually the hint values are supposed to be different).

And this is a priority when you have 2 components with the same hint BUT also the same role (basically an override). Contrary to getRoleTypePriority() which is for any role hint, this one is not for components with any role type. It’s not symmetrical.

I find that getOverridePriority() would be much simpler to understand. The priority could be named simply getPriority() or getCollectionPriority(). Even getRoleTypePriority() would be ok to me since if we have getOverridePriority(), we know what the other one is about.

Anyway, I don’t want to block so I’m +0 for the naming proposal.

Several points:

  1. Why do we need an annotation (vs just the info in the ComponentDescriptor + components.txt)?
  2. Sergiu had a comment about the fact that the priority should be configuration and not code (ie could be changed without having to recompile). Not sure what to think of it.
  3. Is the goal to deprecate the Priority feature for Macros? Same question for Transformation and other components where we have implemented priorities already. I don’t see an easy backward-compatible way to migrate to a new Priority annotation.
  4. For MandatoryDocumentInitializer, is the idea to remove the custom code in initializeMandatoryDocuments (at xwiki-platform/XWiki.java at a75937a9951bb659fca7d25186114374c7ee5889 · xwiki/xwiki-platform · GitHub) since Utils.getComponentList(MandatoryDocumentInitializer.class) should order be ordered?

ok fo me if we’re ok on points 1) and 2) above.

I haven’t thought enough to provide a good answer but what you say seems to make sense to me.

Hard to decide. Slight preference for cross namespace without thinking too much.

Thanks

#18

Answering to myself: this is not for the override but only for the role type priority (ie for list/map order).

#19

It does not really have much to do with EM in practice. This needs to be taken into account by the CM. It’s not only about registration, but also about taking into account the priority cross namespaces. We can even imagine to keep remembering all component and not only the highest priority at a specific namespace level even in the case of role hint priority so that when one is unregistered another one can take its place (but I don’t plan to implement that just yet, that’s improvement for the future).

We already have several components that we sort (you even cite several yourself), but they implement their own system, which ideally should not be the case. On the future use cases side of things, as mentioned in this thread several times and in the past: we need to be able to sort events listeners.

Yes, it’s the priority among the registered components having the same role type.

I don’t agree with that. Most of the time, the need is to make sure that some component is used early, late, or before/after some other specific component, because that’s how it was designed and that’s a developer decision (and especially something that might change with an upgrade because the design changed). Being able to change the priority of the component is just another use case (and probably not a good idea except in very specific and most probably rare cases).

Eventually, but I don’t have this kind of time right now.

As usual in this case, it means having both systems running (meaning that the CM gives you sorted components according to @Priority and then this is sorted again, but according to whatever legacy sorting metadata this component role type used to rely on).

Yes, if we go for the Priority annotation, then MandatoryDocumentInitializer’s specific implementation becomes a duplicate that we can simply remove.

#20

Interesting indeed, as right now, it could be considered a bug that if you overwrite and then uninstall the extension bringing the overwrite you find that XWiki doesn’t have any impl anymore for that component role/hint…

Indeed, not sure why I didn’t think about this when writing my answer :wink:

Note that the meaning of the @Priority annotation will be different for each component type. For macros for example, it means the rendering execution order when doing some XDOM rendering. How do we make that explicit when reading the code? One idea is to be careful to add that info in the javadoc of the component interface.

Yes, I agree too.

Thanks